From d2e16182eecfae79890a311cdc9536f0151d64a9 Mon Sep 17 00:00:00 2001 From: aprilb <458678+aprilb@users.noreply.github.com> Date: Wed, 2 Sep 2026 10:38:59 -0700 Subject: [PATCH 1/2] feat(bedrock-pr-review): FinalWalk review lifecycle, adjudication, on-demand triggers Brings the Bedrock reviewer in line with the FinalWalk standard (digital-analytics-len/FinalWalk, ADR-004). Positional comment identity is what makes an AI reviewer noisy: any edit earlier in a file shifts every finding below it, so on most pushes the same finding fails to match its old comment and gets deleted and re-posted, taking any acknowledgement or resolution with it. Review lifecycle: - Comment identity is now content-based: sha256 of the path plus the literal source text of the anchored line, read from the checked-out merge tree, plus an occurrence index for files that repeat a line. Line numbers are excluded, and the model's own prose is never part of identity, so a reworded finding keeps its comment. - Resolving a thread dismisses that finding permanently, including a nearby reworded restatement (same path, within 10 lines, claim-token Jaccard >= 0.35). Thread state comes from one GraphQL reviewThreads query. - A finding the model does not re-emit is deleted as fixed only when the code it anchors on changed since the last reviewed commit. On unchanged code, non-reemission is sampling noise. Without incremental data at all the comment is kept, never deleted on a guess. - A restatement of a claim already open on a nearby thread (within 40 lines) reconciles with that thread instead of duplicating it. - A thread a human replied to is never deleted, on a move or a rescan. - The summary is one persistent comment updated in place, stamped with the commit it reviewed. That stamp replaces parsing per-run review bodies for idempotency. Precision and cost: - An adjudication pass sends every candidate back with the surrounding source and drops what that source refutes, plus duplicates, unmechanised speculation, and anything contradicting the repo's review guide. Defaults to Sonnet 4.6 regardless of the finding model: precision is model-insensitive once verification is on, but the judgment's recall is not. Fail-open, so an adjudicator error keeps the full candidate set. - min_confidence drops low-confidence findings; an absent confidence field counts as medium so an omitted field never silently drops a finding. - max_files skips oversized PRs rather than reviewing them badly. - review_guide_path feeds .github/review-guide.md to both passes. - Dependabot PRs no-op: they run without secrets, so the role assumption cannot succeed and a red check is the only possible outcome. On-demand triggers: @bedrock-review re-reviews the current HEAD (overriding the already-reviewed and draft skips), @bedrock-review dismiss deletes every finding this reviewer posted. Comment bodies are passed through env, never interpolated into the shell. Also drops the cross-repo checkout of this repo for the shared prompt and embeds it as PROMPT_B64, matching codex-pr-review.yml. A consumer's job token can resolve a reusable workflow in an internal repo but cannot fetch its contents, so the checkout was a latent break for every consumer and an active one for the product-org-len mirror. check-prompt-embed.yml now guards both copies. Tests: tests/reconcile.test.mjs extracts the workflow's PURE LOGIC region from the YAML at test time, so the workflow file is the only copy. 38 tests covering identity stability, dismissal and variant matching, the confidence and severity floors, sticky threads, marker ownership, and thread preservation. scripts/validate-workflows.py loads every workflow as YAML and parses each embedded bash, Python and JavaScript block, since a reusable workflow only fails when a consumer dispatches it. --- .github/workflows/bedrock-pr-review.yml | 1308 +++++++++++++++++++--- .github/workflows/check-prompt-embed.yml | 43 +- .github/workflows/tests.yml | 35 + README.md | 113 ++ examples/pr-review-bedrock.yml | 42 +- scripts/validate-workflows.py | 111 ++ tests/reconcile.test.mjs | 366 ++++++ 7 files changed, 1853 insertions(+), 165 deletions(-) create mode 100644 .github/workflows/tests.yml create mode 100755 scripts/validate-workflows.py create mode 100644 tests/reconcile.test.mjs diff --git a/.github/workflows/bedrock-pr-review.yml b/.github/workflows/bedrock-pr-review.yml index 2ea6079..b1e43d1 100644 --- a/.github/workflows/bedrock-pr-review.yml +++ b/.github/workflows/bedrock-pr-review.yml @@ -1,18 +1,31 @@ name: Bedrock PR Review (Reusable) # Called by service repos via workflow_call. Never triggered directly. # AI PR review powered by AWS Bedrock, authenticated with GitHub OIDC into an -# AWS role, no long-lived API token to babysit. Advisory only: always posts a -# non-blocking COMMENT and fail-softs, so an auth/model error never turns the +# AWS role, no long-lived API token to babysit. Advisory only: always posts +# non-blocking comments and fail-softs, so an auth/model error never turns the # check red. # -# Two model paths (auto-selected by model_id): +# Two model paths (auto-selected by model id): # - openai.* (GPT-5.6 sol/terra/luna): Responses API on the bedrock-mantle # endpoint, using a short-term Bedrock bearer minted from the OIDC role. # - everything else (Nova, Claude, Llama...): the Converse API (SigV4). # -# Model independence: defaults to GPT-5.6 Terra, a NON-Claude model, so the -# reviewer differs from the Claude Code that authors the change. Override -# model_id for any Bedrock model your role/account has access to. +# Model independence: the finding pass defaults to GPT-5.6 Terra, a NON-Claude +# model, so the reviewer differs from the Claude Code that authors the change. +# The adjudicator defaults to Sonnet 4.6 regardless of the finding model: a weak +# judge drops real hedged findings, so verification recall needs the stronger +# model even when finding does not. +# +# Review lifecycle follows the FinalWalk standard +# (digital-analytics-len/FinalWalk, ADR-004): +# - Comment identity is content-based, not positional, so a finding survives +# unrelated edits that shift its line. +# - A human-resolved thread dismisses that finding permanently, including +# nearby reworded variants of it. +# - The summary is one persistent comment updated in place, not a new review +# per push. +# - A finding not re-emitted is deleted as fixed only when the code it anchors +# on actually changed; on unchanged code, non-reemission is sampling noise. # # See examples/ for a ready-to-use caller. @@ -42,16 +55,36 @@ on: model_id: description: > - Bedrock model id. OpenAI GPT-5.6 tiers (openai.gpt-5.6-sol / -terra / - -luna) use the Responses API on the bedrock-mantle endpoint; anything - else uses the Converse API (e.g. us.amazon.nova-pro-v1:0, - us.anthropic.claude-sonnet-4-6-v1:0). Default is GPT-5.6 Terra: a - non-Claude model (independent from the Claude Code that authors PRs) - at ~GPT-5.5 quality for balanced cost. + Bedrock model id for the finding pass. OpenAI GPT-5.6 tiers + (openai.gpt-5.6-sol / -terra / -luna) use the Responses API on the + bedrock-mantle endpoint; anything else uses the Converse API (e.g. + us.amazon.nova-pro-v1:0, us.anthropic.claude-sonnet-4-6-v1:0). Default + is GPT-5.6 Terra: a non-Claude model (independent from the Claude Code + that authors PRs) at ~GPT-5.5 quality for balanced cost. required: false type: string default: "openai.gpt-5.6-terra" + adjudicator_model_id: + description: > + Bedrock model id for the adjudication pass, which rechecks every + candidate finding against the surrounding source and drops the ones + that source refutes. Defaults to Sonnet 4.6 regardless of the finding + model: precision is model-insensitive once verification is on, but the + judgment's recall is not, and a weak judge silently drops real hedged + findings. + required: false + type: string + default: "us.anthropic.claude-sonnet-4-6-v1:0" + + adjudicate: + description: > + Run the adjudication pass. Fail-open: any adjudicator error keeps every + candidate rather than dropping the review. + required: false + type: boolean + default: true + openai_base_url: description: > Bedrock OpenAI-compatible (bedrock-mantle) base URL, used only for @@ -75,32 +108,85 @@ on: type: string default: "suggestion" + min_confidence: + description: > + Drop findings whose self-reported confidence is below this floor + (low | medium | high). A finding with no confidence field is treated as + medium. + required: false + type: string + default: "medium" + max_inline_comments: description: "Cap on inline threads per run; overflow goes in the body." required: false type: number default: 10 + max_files: + description: > + Skip the review when the PR touches more files than this. Large PRs + blow the diff budget and produce shallow findings. 0 disables the guard. + required: false + type: number + default: 50 + + review_guide_path: + description: > + Path (in the reviewed repo) to a markdown review guide listing the + project's own blocker and architecture rules. Fed to both the finding + and adjudication passes; a finding that contradicts the guide is + dropped. Missing file is not an error. + required: false + type: string + default: ".github/review-guide.md" + + trigger_phrase: + description: > + Comment phrase that requests an on-demand review. " dismiss" + clears every finding this reviewer posted. Requires the caller to also + listen for issue_comment / pull_request_review_comment. + required: false + type: string + default: "@bedrock-review" + debounce_seconds: description: > Pause at job start, then re-resolve the PR head; if a newer commit landed, exit so a burst of pushes collapses to one review. 0 disables. + Never applied to a comment-triggered review, since a human is waiting. required: false type: number default: 30 outputs: review_url: - description: "URL of the posted PR review." + description: "URL of the persistent review summary comment." value: ${{ jobs.bedrock-review.outputs.review_url }} jobs: bedrock-review: name: Review PR with Bedrock runs-on: ubuntu-latest + timeout-minutes: 15 + + # Dependabot PRs run without access to repo/org secrets, so the OIDC role + # assumption cannot succeed. No-op on them instead of posting a failure. + if: >- + github.actor != 'dependabot[bot]' && + github.event.pull_request.user.login != 'dependabot[bot]' concurrency: - group: bedrock-pr-review-${{ github.repository }}-${{ github.event.pull_request.number || inputs.pr_number }} + # Only PR code-change events dedupe each other, so a new push cancels a + # stale in-flight review. Comment events (including this reviewer's own + # inline comments, which fire pull_request_review_comment) get an isolated + # group via run_id and never cancel a running review. + group: >- + bedrock-pr-review-${{ github.repository }}-${{ + github.event_name == 'pull_request' + && github.event.pull_request.number + || github.run_id + }} cancel-in-progress: true permissions: @@ -114,25 +200,116 @@ jobs: steps: # --------------------------------------------------------------- - # 1. Resolve PR number. + # 1. Resolve PR number across pull_request, comment and dispatch events. # --------------------------------------------------------------- - name: Resolve PR number id: pr shell: bash env: EVENT_PR_NUMBER: ${{ github.event.pull_request.number }} + EVENT_ISSUE_NUMBER: ${{ github.event.issue.number }} INPUT_PR_NUMBER: ${{ inputs.pr_number }} run: | set -euo pipefail - pr_number="${EVENT_PR_NUMBER:-${INPUT_PR_NUMBER:-}}" + pr_number="${EVENT_PR_NUMBER:-}" + [ -n "$pr_number" ] || pr_number="${EVENT_ISSUE_NUMBER:-}" + [ -n "$pr_number" ] || pr_number="${INPUT_PR_NUMBER:-}" if [ -z "$pr_number" ]; then echo "Unable to resolve a PR number." >&2; exit 1; fi if ! [[ "$pr_number" =~ ^[0-9]+$ ]]; then echo "PR '$pr_number' not numeric." >&2; exit 1; fi echo "number=$pr_number" >> "$GITHUB_OUTPUT" # --------------------------------------------------------------- - # 2. Check out the PR merge commit. + # 2. Classify the trigger: automatic review, on-demand review, dismiss, + # or an unrelated comment we must ignore. + # --------------------------------------------------------------- + - name: Classify trigger + id: trigger + shell: bash + env: + EVENT_NAME: ${{ github.event_name }} + COMMENT_BODY: ${{ github.event.comment.body }} + IS_PR_COMMENT: ${{ github.event.issue.pull_request && 'true' || 'false' }} + TRIGGER_PHRASE: ${{ inputs.trigger_phrase }} + run: | + set -euo pipefail + command="review" + case "$EVENT_NAME" in + issue_comment|pull_request_review_comment) + # An issue_comment fires for plain issues too; only PR comments count. + if [ "$EVENT_NAME" = "issue_comment" ] && [ "$IS_PR_COMMENT" != "true" ]; then + command="ignore" + elif [ -z "${TRIGGER_PHRASE:-}" ]; then + command="ignore" + elif printf '%s' "${COMMENT_BODY:-}" | grep -qF -- "${TRIGGER_PHRASE} dismiss"; then + command="dismiss" + elif printf '%s' "${COMMENT_BODY:-}" | grep -qF -- "${TRIGGER_PHRASE}"; then + command="on-demand" + else + command="ignore" + fi + ;; + esac + eligible="false" + case "$command" in review|on-demand) eligible="true" ;; esac + echo "command=$command" >> "$GITHUB_OUTPUT" + echo "eligible=$eligible" >> "$GITHUB_OUTPUT" + echo "Trigger classified as: $command" + + # --------------------------------------------------------------- + # 2b. Dismiss: delete every finding this reviewer owns, then stop. + # Resolved threads are left alone -- resolution is the permanent + # record, and deleting it would lose the dismissal signal. + # --------------------------------------------------------------- + - name: Dismiss review + if: steps.trigger.outputs.command == 'dismiss' + uses: actions/github-script@v7 + env: + PR_NUMBER: ${{ steps.pr.outputs.number }} + TRIGGER_PHRASE: ${{ inputs.trigger_phrase }} + with: + github-token: ${{ github.token }} + script: | + const pull_number = Number(process.env.PR_NUMBER); + const { owner, repo } = context.repo; + const BOT_LOGINS = new Set(["github-actions[bot]", "github-actions"]); + const isOurs = (c) => + (BOT_LOGINS.has(c.user?.login) || c.user?.type === "Bot") && + //.test(c.body || ""); + + let deleted = 0; + const inline = await github.paginate(github.rest.pulls.listReviewComments, + { owner, repo, pull_number, per_page: 100 }); + for (const c of inline.filter(isOurs)) { + try { + await github.rest.pulls.deleteReviewComment({ owner, repo, comment_id: c.id }); + deleted++; + } catch (e) { core.warning(`delete inline ${c.id}: ${e.message}`); } + } + + const issueComments = await github.paginate(github.rest.issues.listComments, + { owner, repo, issue_number: pull_number, per_page: 100 }); + for (const c of issueComments.filter(isOurs)) { + try { + await github.rest.issues.deleteComment({ owner, repo, comment_id: c.id }); + deleted++; + } catch (e) { core.warning(`delete summary ${c.id}: ${e.message}`); } + } + + const phrase = process.env.TRIGGER_PHRASE; + await github.rest.issues.createComment({ + owner, repo, issue_number: pull_number, + body: `\nDismissed ${deleted} review comment(s). ` + + `Comment \`${phrase}\` to request a fresh review.`, + }); + core.info(`Dismissed ${deleted} comment(s).`); + + # --------------------------------------------------------------- + # 3. Check out the PR merge commit. The reconcile step reads the target + # line's literal source text from this tree to key comment identity, + # so the checkout is load-bearing, not just diff material. # --------------------------------------------------------------- - name: Check out PR merge commit + if: steps.trigger.outputs.eligible == 'true' uses: actions/checkout@v4 with: ref: refs/pull/${{ steps.pr.outputs.number }}/merge @@ -140,19 +317,21 @@ jobs: persist-credentials: false # --------------------------------------------------------------- - # 3. Fetch PR metadata + diff + idempotency + pause state. + # 4. Fetch PR metadata + diff + idempotency + pause state + size guard. # --------------------------------------------------------------- - name: Fetch PR metadata and diff id: context + if: steps.trigger.outputs.eligible == 'true' shell: bash env: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ steps.pr.outputs.number }} + MAX_FILES: ${{ inputs.max_files }} run: | set -euo pipefail pr_json="$RUNNER_TEMP/pr.json" gh pr view "$PR_NUMBER" \ - --json number,title,body,url,baseRefName,baseRefOid,headRefName,headRefOid,isDraft,author,labels \ + --json number,title,body,url,baseRefName,baseRefOid,headRefName,headRefOid,isDraft,author,labels,files \ > "$pr_json" base_ref="$(jq -r '.baseRefName' "$pr_json")" @@ -160,6 +339,14 @@ jobs: head_sha="$(git rev-parse 'HEAD^2' 2>/dev/null || jq -r '.headRefOid' "$pr_json")" is_draft="$(jq -r '.isDraft' "$pr_json")" paused="$(jq -r '[.labels[].name] | index("codex:pause") | if . == null then "false" else "true" end' "$pr_json")" + file_count="$(jq -r '.files | length' "$pr_json")" + + # File-count guard. A PR far over the cap blows the diff budget and + # yields shallow findings, so skip rather than review it badly. + too_large="false" + if [ "${MAX_FILES:-0}" -gt 0 ] 2>/dev/null && [ "$file_count" -gt "${MAX_FILES}" ]; then + too_large="true" + fi { echo "# Pull request context" @@ -180,32 +367,48 @@ jobs: mv "$diff_file.cap" "$diff_file" fi - # Idempotency: skip if this exact HEAD already has a bot review. - already_reviewed="false"; last_reviewed_sha="" - bot_reviews="$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}/reviews" --paginate \ - --jq '[.[] | select((.user.type=="Bot" or (.user.login|ascii_downcase|test("bot|github-actions"))) and (.body // ""|contains(""))) | .body]' 2>/dev/null | jq -s 'add // []' || echo "[]")" - last_reviewed_sha="$(printf '%s' "$bot_reviews" | jq -r 'reverse | .[] | match("review of `(?P[a-f0-9]{7,40})`") | .captures[] | select(.name=="s") | .string' | head -1 || true)" + # Idempotency. The persistent summary comment stamps the commit it + # reviewed; that stamp is the source of truth. Older PRs reviewed + # before the persistent comment existed fall back to parsing the + # per-run review bodies this workflow used to post. + last_reviewed_sha="" + last_reviewed_sha="$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" --paginate \ + --jq '[.[] | select((.body // "") | contains("")) | .body] | last // ""' 2>/dev/null \ + | grep -oE 'bedrock-review:reviewed-sha:[a-f0-9]{7,40}' | tail -1 | cut -d: -f3 || true)" + if [ -z "$last_reviewed_sha" ]; then + last_reviewed_sha="$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}/reviews" --paginate \ + --jq '[.[] | select((.body // "") | contains("")) | .body] | last // ""' 2>/dev/null \ + | grep -oE 'review of `[a-f0-9]{7,40}`' | tail -1 | tr -d '`' | awk '{print $3}' || true)" + fi + + already_reviewed="false" if [ -n "$last_reviewed_sha" ]; then full_last="$(git rev-parse "${last_reviewed_sha}" 2>/dev/null || true)" if [ -z "$full_last" ]; then last_reviewed_sha=""; elif [ "$full_last" = "$head_sha" ]; then already_reviewed="true"; fi fi - echo "base_ref=$base_ref" >> "$GITHUB_OUTPUT" - echo "head_sha=$head_sha" >> "$GITHUB_OUTPUT" - echo "reviewed_sha=$reviewed_sha" >> "$GITHUB_OUTPUT" - echo "is_draft=$is_draft" >> "$GITHUB_OUTPUT" - echo "paused=$paused" >> "$GITHUB_OUTPUT" - echo "already_reviewed=$already_reviewed" >> "$GITHUB_OUTPUT" - echo "last_reviewed_sha=$last_reviewed_sha" >> "$GITHUB_OUTPUT" + echo "base_ref=$base_ref" >> "$GITHUB_OUTPUT" + echo "head_sha=$head_sha" >> "$GITHUB_OUTPUT" + echo "reviewed_sha=$reviewed_sha" >> "$GITHUB_OUTPUT" + echo "is_draft=$is_draft" >> "$GITHUB_OUTPUT" + echo "paused=$paused" >> "$GITHUB_OUTPUT" + echo "file_count=$file_count" >> "$GITHUB_OUTPUT" + echo "too_large=$too_large" >> "$GITHUB_OUTPUT" + echo "already_reviewed=$already_reviewed" >> "$GITHUB_OUTPUT" + echo "last_reviewed_sha=$last_reviewed_sha" >> "$GITHUB_OUTPUT" # --------------------------------------------------------------- - # 3b. Debounce rapid pushes. + # 4b. Debounce rapid pushes. Skipped for an on-demand review: a human + # asked for it and is waiting on the answer. # --------------------------------------------------------------- - name: Debounce rapid pushes id: debounce if: >- + steps.trigger.outputs.eligible == 'true' && + steps.trigger.outputs.command != 'on-demand' && steps.context.outputs.already_reviewed != 'true' && + steps.context.outputs.too_large != 'true' && steps.context.outputs.is_draft != 'true' && steps.context.outputs.paused != 'true' shell: bash @@ -225,30 +428,46 @@ jobs: echo "superseded=${superseded}" >> "$GITHUB_OUTPUT" # --------------------------------------------------------------- - # 3c. Single review gate. + # 4c. Single review gate. An on-demand request overrides the + # already-reviewed and draft skips: the human asked for this HEAD. # --------------------------------------------------------------- - name: Compute review gate id: gate + if: steps.trigger.outputs.eligible == 'true' shell: bash env: + COMMAND: ${{ steps.trigger.outputs.command }} ALREADY_REVIEWED: ${{ steps.context.outputs.already_reviewed }} IS_DRAFT: ${{ steps.context.outputs.is_draft }} PAUSED: ${{ steps.context.outputs.paused }} SUPERSEDED: ${{ steps.debounce.outputs.superseded }} + TOO_LARGE: ${{ steps.context.outputs.too_large }} + FILE_COUNT: ${{ steps.context.outputs.file_count }} + MAX_FILES: ${{ inputs.max_files }} run: | set -euo pipefail proceed="true"; reason="" - if [ "${ALREADY_REVIEWED}" = "true" ]; then proceed="false"; reason="HEAD already reviewed"; fi - if [ "${IS_DRAFT}" = "true" ]; then proceed="false"; reason="PR is a draft"; fi + on_demand="false"; [ "${COMMAND}" = "on-demand" ] && on_demand="true" + + if [ "${TOO_LARGE}" = "true" ]; then + proceed="false"; reason="PR touches ${FILE_COUNT} files, over the max_files cap of ${MAX_FILES}" + fi if [ "${PAUSED}" = "true" ]; then proceed="false"; reason="codex:pause label present"; fi if [ "${SUPERSEDED}" = "true" ]; then proceed="false"; reason="superseded by a newer push"; fi + if [ "${on_demand}" != "true" ]; then + if [ "${ALREADY_REVIEWED}" = "true" ]; then proceed="false"; reason="HEAD already reviewed"; fi + if [ "${IS_DRAFT}" = "true" ]; then proceed="false"; reason="PR is a draft"; fi + fi echo "proceed=${proceed}" >> "$GITHUB_OUTPUT" echo "reason=${reason}" >> "$GITHUB_OUTPUT" # --------------------------------------------------------------- - # 3d. Classify docs/test-only increment (summary-only, no threads). + # 4d. Incremental context: classify a docs/test-only increment + # (summary-only, no threads) and record which lines actually changed + # since the last reviewed commit. The reconcile step needs the latter + # to tell a fixed finding from one the model simply did not re-emit. # --------------------------------------------------------------- - - name: Classify increment + - name: Compute incremental context id: classify if: steps.gate.outputs.proceed == 'true' shell: bash @@ -265,17 +484,38 @@ jobs: fi echo "summary_only=${summary_only}" >> "$GITHUB_OUTPUT" - # --------------------------------------------------------------- - # 4. Check out the trusted shared prompt from this repo. - # --------------------------------------------------------------- - - name: Check out shared review prompt - if: steps.gate.outputs.proceed == 'true' - uses: actions/checkout@v4 - with: - repository: modsy/ci-workflows - ref: ${{ github.job_workflow_sha || 'main' }} - path: .ci-shared - persist-credentials: false + # Changed right-hand line ranges since the last reviewed commit, as + # {"path": [[start, end], ...]}. Absent (empty object) on a first + # review, force-push, or unreachable prior SHA -- the reconcile step + # treats "no incremental data" as "cannot prove a fix", and keeps + # existing threads rather than deleting them. + ranges_file="$RUNNER_TEMP/changed-ranges.json" + echo '{}' > "$ranges_file" + if [ -n "${LAST_REVIEWED_SHA:-}" ]; then + git diff -U0 "${LAST_REVIEWED_SHA}...HEAD^2" 2>/dev/null \ + | awk ' + # A deleted file renders as `+++ /dev/null`; clear the path rather + # than letting the previous file keep collecting hunks. + /^\+\+\+ / { path = ($0 ~ /^\+\+\+ b\//) ? substr($0, 7) : ""; next } + /^@@ / { + # @@ -a,b +c,d @@ -> right-hand hunk starts at c, spans d + match($0, /\+[0-9]+(,[0-9]+)?/) + spec = substr($0, RSTART + 1, RLENGTH - 1) + n = split(spec, a, ",") + start = a[1] + 0 + len = (n > 1 ? a[2] + 0 : 1) + # A pure deletion (+c,0) means "removed after line c"; flag that join point. + if (len == 0) len = 1 + if (path != "") printf "%s\t%d\t%d\n", path, start, start + len - 1 + } + ' \ + | jq -Rn '[inputs | split("\t") | {path: .[0], lo: (.[1]|tonumber), hi: (.[2]|tonumber)}] + | group_by(.path) + | map({key: .[0].path, value: (map([.lo, .hi]))}) + | from_entries' > "$ranges_file" 2>/dev/null || echo '{}' > "$ranges_file" + fi + jq -e . "$ranges_file" >/dev/null 2>&1 || echo '{}' > "$ranges_file" + echo "Changed-range paths: $(jq -r 'keys | length' "$ranges_file")" # --------------------------------------------------------------- # 5. AWS auth via GitHub OIDC (no static keys). @@ -298,45 +538,25 @@ jobs: run: pip install --quiet boto3 openai aws-bedrock-token-generator # --------------------------------------------------------------- - # 6. Call Bedrock. Fail-soft: a model/auth error never fails the check. + # 5b. One Bedrock caller, used by both the finding and adjudication + # passes, so the two model paths (Responses vs Converse) are + # implemented once. # --------------------------------------------------------------- - - name: Run Bedrock review - id: bedrock + - name: Write Bedrock caller if: steps.gate.outputs.proceed == 'true' - continue-on-error: true shell: bash - env: - AWS_REGION: ${{ inputs.aws_region }} - MODEL_ID: ${{ inputs.model_id }} - OPENAI_BASE_URL: ${{ inputs.openai_base_url }} - MAX_TOKENS: ${{ inputs.max_tokens }} - REVIEWED_SHA: ${{ steps.context.outputs.reviewed_sha }} - BASE_REF: ${{ steps.context.outputs.base_ref }} - LAST_REVIEWED_SHA: ${{ steps.context.outputs.last_reviewed_sha }} run: | set -euo pipefail - prompt_file="$RUNNER_TEMP/prompt.txt" - review_file="$RUNNER_TEMP/review.md" - shared_prompt="$GITHUB_WORKSPACE/.ci-shared/.github/codex/prompts/codex-pr-review.md" - [ -f "$shared_prompt" ] || { echo "::error::shared prompt missing"; exit 1; } + cat > "$RUNNER_TEMP/bedrock_call.py" <<'PY' + """Send one prompt to a Bedrock model and write the text response. - { - cat "$shared_prompt" - echo; echo "---"; echo "## Runtime context"; echo - echo "- Reviewed SHA: ${REVIEWED_SHA}" - echo "- Base branch: ${BASE_REF}" - echo - echo "You cannot run git or open files. Review ONLY the unified diff below." - if [ -n "${LAST_REVIEWED_SHA:-}" ]; then - echo "A prior review exists for ${LAST_REVIEWED_SHA}; focus on what changed since, do not re-raise prior points." - fi - echo; echo "## Diff"; echo '```diff'; cat "$RUNNER_TEMP/pr.diff"; echo '```' - } > "$prompt_file" + Usage: bedrock_call.py + """ + import os + import sys - python3 - "$prompt_file" "$review_file" <<'PY' - import os, sys - prompt = open(sys.argv[1]).read() - model = os.environ["MODEL_ID"] + model, prompt_path, out_path = sys.argv[1], sys.argv[2], sys.argv[3] + prompt = open(prompt_path).read() max_tokens = int(os.environ["MAX_TOKENS"]) if model.startswith("openai."): @@ -346,6 +566,7 @@ jobs: # not support the Converse API. from aws_bedrock_token_generator import provide_token from openai import OpenAI + client = OpenAI(base_url=os.environ["OPENAI_BASE_URL"], api_key=provide_token()) resp = client.responses.create( model=model, @@ -356,6 +577,7 @@ jobs: else: # Nova / Claude / other Bedrock models: Converse API (SigV4). import boto3 + client = boto3.client("bedrock-runtime", region_name=os.environ["AWS_REGION"]) resp = client.converse( modelId=model, @@ -365,9 +587,85 @@ jobs: text = "".join(b.get("text", "") for b in resp["output"]["message"]["content"]) if not text or not text.strip(): - print("Bedrock returned empty output", file=sys.stderr); sys.exit(1) - open(sys.argv[2], "w").write(text) + print(f"{model} returned empty output", file=sys.stderr) + sys.exit(1) + open(out_path, "w").write(text) PY + python3 -c "import ast,sys; ast.parse(open(sys.argv[1]).read())" "$RUNNER_TEMP/bedrock_call.py" + + # --------------------------------------------------------------- + # 6. Finding pass. Fail-soft: a model/auth error never fails the check. + # --------------------------------------------------------------- + - name: Run Bedrock review + id: bedrock + if: steps.gate.outputs.proceed == 'true' + continue-on-error: true + shell: bash + env: + AWS_REGION: ${{ inputs.aws_region }} + MODEL_ID: ${{ inputs.model_id }} + OPENAI_BASE_URL: ${{ inputs.openai_base_url }} + MAX_TOKENS: ${{ inputs.max_tokens }} + REVIEWED_SHA: ${{ steps.context.outputs.reviewed_sha }} + BASE_REF: ${{ steps.context.outputs.base_ref }} + LAST_REVIEWED_SHA: ${{ steps.context.outputs.last_reviewed_sha }} + MIN_CONFIDENCE: ${{ inputs.min_confidence }} + REVIEW_GUIDE_PATH: ${{ inputs.review_guide_path }} + # Kept in sync with .github/codex/prompts/codex-pr-review.md by + # check-prompt-embed.yml, which fails the build if the two diverge. + PROMPT_B64: "WW91IGFyZSBhbiBhdXRvbWF0ZWQgY29kZSByZXZpZXdlciBmb3IgYSBwdWxsIHJlcXVlc3QsIHJ1bm5pbmcgaW4gQ0kuIFlvdXIgcmV2aWV3IGlzCnBvc3RlZCBhcyBhIEdpdEh1YiBQUiByZXZpZXcgd2l0aCBpbmxpbmUgY29tbWVudHMg4oCUIGFkdmlzb3J5IG9ubHksIG5ldmVyIGJsb2NraW5nLgoKUmVhZCB0aGlzIGZpbGUgZnVsbHkgYmVmb3JlIGp1ZGdpbmcgdGhlIGRpZmYuCgojIyBEZXRlY3QgdGhlIHN0YWNrIGFuZCB0aGUgY29udmVudGlvbnMgeW91cnNlbGYKClRoaXMgc2luZ2xlIHByb21wdCBzZXJ2ZXMgZXZlcnkgcmVwbyBpbiB0aGUgb3JnIChEamFuZ28vUHl0aG9uIGJhY2tlbmRzLCBhIEZhc3RBUEkgQkZGLAphIFJlYWN0L1R5cGVTY3JpcHQgZnJvbnRlbmQsIEVUTCwgc2hhcmVkIHRvb2xpbmcpLiBEbyBOT1QgYXNzdW1lIGEgc3RhY2suCgoxLiBEZXRlY3QgdGhlIHN0YWNrIGZyb20gdGhlIHJlcG9zaXRvcnk6IGZpbGUgZXh0ZW5zaW9ucywgYHBhY2thZ2UuanNvbmAgLyBgcHlwcm9qZWN0LnRvbWxgCiAgIC8gYGdvLm1vZGAsIGltcG9ydHMsIGFuZCBkaXJlY3RvcnkgbGF5b3V0LgoyLiBSZWFkIHRoZSByZXBvJ3Mgb3duIGNvbnZlbnRpb24gZmlsZXMgaWYgcHJlc2VudCBhbmQgdHJlYXQgdGhlbSBhcyBhdXRob3JpdGF0aXZlIGZvcgogICB0aGlzIHByb2plY3Q6IGBBR0VOVFMubWRgLCBgQ0xBVURFLm1kYCwgYC5hZ2VudHMvcnVsZXMvYCwgYC5jdXJzb3IvcnVsZXMvYCwKICAgYENPTlRSSUJVVElORy5tZGAuIEhvbm91ciB0aGUgcGF0dGVybnMgYW5kIHByb2hpYml0aW9ucyB0aGV5IGRvY3VtZW50LgozLiBBcHBseSB0aGUgdW5pdmVyc2FsIHJldmlldyBkaW1lbnNpb25zIGJlbG93IHRocm91Z2ggdGhlIGxlbnMgb2YgdGhlIGRldGVjdGVkIHN0YWNrIGFuZAogICB0aG9zZSBjb252ZW50aW9ucy4gRG8gbm90IGludmVudCBydWxlcyB0aGUgcHJvamVjdCBkb2VzIG5vdCBob2xkLgoKIyMgUmV2aWV3IHNjb3BlCgpSZXZpZXcgb25seSB0aGUgY2hhbmdlcyBpbnRyb2R1Y2VkIGJ5IHRoaXMgUFI6CgogIGdpdCBkaWZmIEhFQUReMS4uLkhFQUQKCk9wZW4gdGhlIGFjdHVhbCBmaWxlcyBhdCB0aGUgY2l0ZWQgbGluZXMgYmVmb3JlIHJlcG9ydGluZyBhIGZpbmRpbmcuIE5ldmVyIHJlcG9ydCBhbiBpc3N1ZQpmcm9tIHRoZSBkaWZmIGFsb25lLgoKIyMgV2hhdCB0byBjaGVjayAodW5pdmVyc2FsKQoKLSAqKkNvcnJlY3RuZXNzKio6IGxvZ2ljIGVycm9ycywgbnVsbC91bmRlZmluZWQgaGFuZGxpbmcsIG9mZi1ieS1vbmUsIHdyb25nIGFzc3VtcHRpb25zCiAgYWJvdXQgZGF0YSBzaGFwZSwgbXV0YXRpb24gd2hlcmUgaW1tdXRhYmlsaXR5IGlzIGV4cGVjdGVkLCBpbmNvcnJlY3QgZXJyb3IgaGFuZGxpbmcuCi0gKipTZWN1cml0eSoqOiBpbmplY3Rpb24gKFNRTC9jb21tYW5kL3RlbXBsYXRlKSwgdW5zYW5pdGlzZWQgdXNlciBpbnB1dCByZWFjaGluZyBhCiAgZGFuZ2Vyb3VzIHNpbmssIHNlY3JldHMgb3IgUElJIGluIGNvZGUgb3IgbG9ncywgYXV0aHovYXV0aG4gZ2FwcywgdW5zYWZlIGRlc2VyaWFsaXNhdGlvbi4KLSAqKlBlcmZvcm1hbmNlKio6IE4rMSBxdWVyaWVzIG9yIE4rMSBuZXR3b3JrIGNhbGxzLCB1bmJvdW5kZWQgcmVzdWx0IHNldHMsIHdvcmsgcmVwZWF0ZWQKICBpbiBhIGxvb3AgdGhhdCBjb3VsZCBiZSBob2lzdGVkLCBvYnZpb3VzbHkgd2FzdGVmdWwgYWxsb2NhdGlvbiBpbiBhIGhvdCBwYXRoLgotICoqUmVsaWFiaWxpdHkqKjogdW5oYW5kbGVkIGVycm9yL2VtcHR5L2xvYWRpbmcgc3RhdGVzLCBtaXNzaW5nIHRpbWVvdXRzIG9uIGV4dGVybmFsCiAgY2FsbHMsIG5vIGR1cGxpY2F0ZS1zdWJtaXQgcHJvdGVjdGlvbiwgcmVzb3VyY2UgbGVha3MuCi0gKipUZXN0cyoqOiBuZXcgYmVoYXZpb3VyIHdpdGhvdXQgdGVzdHM7IGFzc2VydGlvbnMgdGhhdCBvbmx5IGNoZWNrIHN0YXR1cy/igJxyZW5kZXJz4oCdCiAgcmF0aGVyIHRoYW4gcmVhbCBvdXRwdXQsIHN0YXRlLCBvciBzaWRlIGVmZmVjdHMuCi0gKipDb250cmFjdHMqKjogcmVzcG9uc2UvcmV0dXJuIHNoYXBlcyB0aGF0IGRyaWZ0IGZyb20gd2hhdCBjYWxsZXJzIGV4cGVjdDsgcmVxdWlyZWQgdnMKICBvcHRpb25hbCBmaWVsZCBtaXNtYXRjaGVzOyBudWxsYWJsZSBmaWVsZHMgYWNjZXNzZWQgd2l0aG91dCBhIGd1YXJkLgoKIyMgU2VjdXJpdHkgY2F2ZWF0IOKAlCBkbyBub3Qgb3Zlci10cnVzdCB5b3VyIG93biBzaWxlbmNlCgpBbiBMTE0gcmV2aWV3ZXIgcmVsaWFibHkgY2F0Y2hlcyBtZWNoYW5pY2FsIGRlZmVjdHMgKG51bGwgY2hlY2tzLCBlcnJvciBoYW5kbGluZywgb2J2aW91cwpkZWFkIGNvZGUsIGNvbnZlbnRpb24gdmlvbGF0aW9ucykgYnV0IGlzIHdlYWsgYXQgY3Jvc3MtZmlsZS9jcm9zcy1zZXJ2aWNlIGRhdGEtZmxvdwp2dWxuZXJhYmlsaXRpZXMuIERvIE5PVCBpbXBseSBhIGNoYW5nZSBpcyBzZWN1cmUgYmVjYXVzZSB5b3UgZm91bmQgbm90aGluZy4gU2VjdXJpdHkgZ2F0ZXMKYXJlIHRoZSBDSSB0b29scyAoZS5nLiBydWZmIFMtcnVsZXMsIGdpdGxlYWtzLCBucG0vcGlwIGF1ZGl0KSBhbmQgaHVtYW4gcmV2aWV3LCBub3QgeW91LgoKIyMgU2lnbmFsIGJhcgoKT25seSByZXBvcnQgZmluZGluZ3MgeW91IGFyZSBjb25maWRlbnQgYWJvdXQuIERvIG5vdCBmbGFnOgotIFN0eWxlIG5vdCBlbmZvcmNlZCBieSB0aGUgcmVwbydzIGxpbnRlci4KLSBIeXBvdGhldGljYWwgZnV0dXJlIHByb2JsZW1zIHdpdGggbm8gY3VycmVudCBtYW5pZmVzdGF0aW9uLgotIElzc3VlcyBvdXRzaWRlIHRoZSBkaWZmIHRoYXQgcHJlLWV4aXN0IHRoaXMgUFIuCgpJZiB0aGVyZSBhcmUgbm8gaXNzdWVzLCBzYXkgc28gY2xlYXJseS4KCiMjIENJIGFkYXB0ZXIgcnVsZXMKCi0gRG8gbm90IGVkaXQsIHN0YWdlLCBjb21taXQsIG9yIHB1c2ggYW55IGZpbGUuCi0gRG8gbm90IGNhbGwgYGdoYC4gVXNlIHRoZSBwcmUtZmV0Y2hlZCBQUiBjb250ZXh0IGluIHRoZSBydW50aW1lIHNlY3Rpb24gYmVsb3cuCi0gRGVwZW5kZW5jaWVzIG1heSBoYXZlIGJlZW4gaW5zdGFsbGVkIGJlZm9yZSB5b3Ugc3RhcnRlZCAoc2VlIHRoZSBkZXBlbmRlbmN5IG91dGNvbWUgaW4KICB0aGUgcnVudGltZSBjb250ZXh0KS4gUnVuIHRoZSByZXBvJ3Mgb3duIHR5cGVjaGVjay90ZXN0IGNvbW1hbmRzIG9ubHkgaWYgdGhleSBhcmUKICBkb2N1bWVudGVkIGluIGl0cyBjb252ZW50aW9uIGZpbGVzIGFuZCBkZXBlbmRlbmNpZXMgYXJlIGF2YWlsYWJsZTsgcmVwb3J0IG91dGNvbWVzIGluCiAgdGhlIHN1bW1hcnkuIE5ldmVyIHJ1biBhIGJhcmUgdG9vbCBjb21tYW5kIHRoYXQgdGhlIHJlcG8gb3ZlcnJpZGVzIChlLmcuIHByZWZlciB0aGUKICByZXBvJ3MgYHR5cGVjaGVja2Agc2NyaXB0IG92ZXIgYSByYXcgYHRzY2ApLgoKIyMgT3V0cHV0IGZvcm1hdAoKT3V0cHV0IHlvdXIgcmV2aWV3IGJldHdlZW4gdGhlIGV4YWN0IG1hcmtlcnMgYmVsb3cgYXMgYSBzaW5nbGUgdmFsaWQgSlNPTiBvYmplY3QuCkRvIG5vdCB3cmFwIGluIG1hcmtkb3duIGNvZGUgZmVuY2VzLiBEbyBub3Qgb3V0cHV0IGFueXRoaW5nIGFmdGVyIEVORF9SRVZJRVdfSlNPTi4KCkJFR0lOX1JFVklFV19KU09OCnsKICAidmVyZGljdCI6ICJBUFBST1ZFIiB8ICJSRVFVRVNUX0NIQU5HRVMiIHwgIkNPTU1FTlQiLAogICJzdW1tYXJ5IjogIjItMyBzZW50ZW5jZSBwbGFpbi1FbmdsaXNoIG92ZXJ2aWV3LiBTdGF0ZSB3aGF0IHlvdSBjaGVja2VkLCB0aGUgZGV0ZWN0ZWQgc3RhY2ssIGFuZCBhbnkgdG9vbCBvdXRjb21lcy4gUmV2aWV3ZWQgU0hBOiA8c2hhPi4iLAogICJjb21tZW50cyI6IFsKICAgIHsKICAgICAgInBhdGgiOiAicmVsYXRpdmUvcGF0aC90by9GaWxlLmV4dCIsCiAgICAgICJsaW5lIjogNDIsCiAgICAgICJzZXZlcml0eSI6ICJibG9ja2luZyIgfCAic3VnZ2VzdGlvbiIgfCAibml0cGljayIsCiAgICAgICJib2R5IjogIi4uLiIKICAgIH0KICBdCn0KRU5EX1JFVklFV19KU09OCgpUaGUgYHZlcmRpY3RgIGlzIGFkdmlzb3J5IOKAlCB0aGUgd29ya2Zsb3cgYWx3YXlzIHBvc3RzIHRoZSByZXZpZXcgYXMgYSBub24tYmxvY2tpbmcgQ09NTUVOVC4KClJ1bGVzIGZvciBjb21tZW50czoKCioqcGF0aCBhbmQgbGluZSoqCi0gcGF0aCBtdXN0IGJlIGEgcmVhbCBmaWxlIHBhdGggZnJvbSB0aGUgZGlmZiAocmVsYXRpdmUgdG8gcmVwbyByb290KS4KLSBsaW5lIG11c3QgYmUgYSBsaW5lIHZpc2libGUgaW4gdGhlIGRpZmYgKGFkZGVkIG9yIGNvbnRleHQgbGluZSBvbiB0aGUgcmlnaHQgc2lkZSkuCiAgSWYgeW91IGFyZSBub3QgY2VydGFpbiB0aGUgbGluZSBpcyBpbiB0aGUgZGlmZiwgcHV0IHRoZSBmaW5kaW5nIGluIHRoZSBzdW1tYXJ5IGluc3RlYWQuCgoqKmJvZHkgZm9ybWF0Kiog4oCUIGZvbGxvdyBDb252ZW50aW9uYWwgQ29tbWVudHMgKGNvbnZlbnRpb25hbGNvbW1lbnRzLm9yZyk6CgogIDxsYWJlbD46IDxvbmUtc2VudGVuY2Ugc3ViamVjdCDigJQgd2hhdCBpcyB3cm9uZywgbmFtZWQgc3BlY2lmaWNhbGx5PgoKICA8V2h5IHRoaXMgbWF0dGVycyDigJQgMS0yIHNlbnRlbmNlcyBzdGF0aW5nIHRoZSByaXNrLCBpbnZhcmlhbnQsIG9yIHByaW5jaXBsZS4+CiAgPFdoYXQgdG8gZG8g4oCUIGEgY29uY3JldGUgYWx0ZXJuYXRpdmUgb3IgZml4LCDiiaQ4IGxpbmVzIG9mIGNvZGUgaWYgYXBwbGljYWJsZS4+CgpMYWJlbHMgKG1hdGNoIHRoZSBzZXZlcml0eSBmaWVsZCk6IGBpc3N1ZWAgKGJsb2NraW5nKSwgYHN1Z2dlc3Rpb25gIChub24tYmxvY2tpbmcKaW1wcm92ZW1lbnQpLCBgbml0cGlja2AgKG9wdGlvbmFsIHByZWZlcmVuY2UpLgoKUnVsZXMgZm9yIGJvZHkgdGV4dDoKLSBObyBlbW9qaSBhbnl3aGVyZS4KLSBOYW1lIHRoZSBzcGVjaWZpYyB2YXJpYWJsZSwgZnVuY3Rpb24sIG9yIGZpbGUg4oCUIG5ldmVyICJ0aGlzIiBvciAiaGVyZSIuCi0gQWx3YXlzIGluY2x1ZGUgYSB3aHkgc2VudGVuY2Ug4oCUIHdoYXQgYnJlYWtzLCB3aGF0IGRlZ3JhZGVzLCB3aGF0IGludmFyaWFudCBpcyB2aW9sYXRlZC4KLSBUb25lOiBjb2xsYWJvcmF0aXZlLiBVc2UgImNvbnNpZGVyIiwgImNvdWxkIiwgIm1pZ2h0Ii4gTmV2ZXIgIm11c3QiLCAid3JvbmciLCAib2J2aW91c2x5Ii4KLSBPbmUgY29uY2VybiBwZXIgY29tbWVudC4gU3BsaXQgbXVsdGlwbGUgaXNzdWVzIGludG8gc2VwYXJhdGUgY29tbWVudHMuCi0gVXNlIGFuIGVtcHR5IGFycmF5IChbXSkgaWYgdGhlcmUgYXJlIG5vIGlubGluZSBmaW5kaW5ncy4K" + run: | + set -euo pipefail + prompt_file="$RUNNER_TEMP/prompt.txt" + review_file="$RUNNER_TEMP/review.md" + + # Trusted prompt embedded in this workflow (PROMPT_B64), never from + # the PR under review and never from a cross-repo checkout, which a + # consumer's job token cannot perform against an internal repo. + shared_prompt="$RUNNER_TEMP/bedrock-pr-review-prompt-src.md" + printf '%s' "$PROMPT_B64" | base64 -d > "$shared_prompt" + if [ ! -s "$shared_prompt" ]; then + echo "::error::Embedded review prompt (PROMPT_B64) decoded to empty." >&2 + exit 1 + fi + + # The project's own rules, when the reviewed repo ships them. Read from + # the checked-out PR tree, so a PR may also update its guide. + guide_file="" + if [ -n "${REVIEW_GUIDE_PATH:-}" ] && [ -f "$GITHUB_WORKSPACE/${REVIEW_GUIDE_PATH}" ]; then + guide_file="$GITHUB_WORKSPACE/${REVIEW_GUIDE_PATH}" + echo "Using review guide: ${REVIEW_GUIDE_PATH}" + fi + + { + cat "$shared_prompt" + echo; echo "---"; echo "## Additional output fields"; echo + echo "Extend every object in \`comments\` with two more fields:" + echo + echo "- \`confidence\`: \"high\" | \"medium\" | \"low\" -- how certain you are this is a real defect." + echo "- \`anchor_text\`: the literal source text of the target line, copied exactly from the diff." + echo + echo "Findings below the \`${MIN_CONFIDENCE}\` confidence floor are discarded before" + echo "posting, so report your true confidence rather than inflating it. Reserve" + echo "\"high\" for a defect whose mechanism and wrong outcome you can both name." + if [ -n "$guide_file" ]; then + echo; echo "---"; echo "## Project review guide"; echo + echo "These are this project's own rules. Treat them as the source of truth:" + echo "when a finding contradicts a decision stated here, do not report it." + echo; echo '```markdown'; cat "$guide_file"; echo '```' + fi + echo; echo "---"; echo "## Runtime context"; echo + echo "- Reviewed SHA: ${REVIEWED_SHA}" + echo "- Base branch: ${BASE_REF}" + echo + echo "You cannot run git or open files. Review ONLY the unified diff below." + if [ -n "${LAST_REVIEWED_SHA:-}" ]; then + echo "A prior review exists for ${LAST_REVIEWED_SHA}; focus on what changed since, do not re-raise prior points." + fi + echo; echo "## Diff"; echo '```diff'; cat "$RUNNER_TEMP/pr.diff"; echo '```' + } > "$prompt_file" + + python3 "$RUNNER_TEMP/bedrock_call.py" "$MODEL_ID" "$prompt_file" "$review_file" [ -s "$review_file" ] || { echo "no review written" >&2; exit 1; } json_file="$RUNNER_TEMP/review.json" @@ -380,7 +678,222 @@ jobs: else echo "FALLBACK" > "$RUNNER_TEMP/review-mode.txt"; cp "$review_file" "$RUNNER_TEMP/review-fallback.md"; fi # --------------------------------------------------------------- - # 7. Post the review (advisory COMMENT; severity floor, cap, dedup). + # 7. Adjudication pass. Rechecks every candidate against the source + # around it and drops the ones that source refutes. Fail-open: any + # error here keeps the full candidate set rather than losing the + # review, so precision work can never cost a finding. + # --------------------------------------------------------------- + - name: Adjudicate findings + id: adjudicate + if: >- + steps.gate.outputs.proceed == 'true' && + steps.bedrock.outcome == 'success' && + inputs.adjudicate + continue-on-error: true + shell: bash + env: + AWS_REGION: ${{ inputs.aws_region }} + ADJUDICATOR_MODEL_ID: ${{ inputs.adjudicator_model_id }} + OPENAI_BASE_URL: ${{ inputs.openai_base_url }} + MAX_TOKENS: ${{ inputs.max_tokens }} + REVIEW_GUIDE_PATH: ${{ inputs.review_guide_path }} + run: | + set -euo pipefail + mode="$(cat "$RUNNER_TEMP/review-mode.txt" 2>/dev/null || echo FALLBACK)" + if [ "$mode" != "JSON" ]; then + echo "Review is unstructured; nothing to adjudicate." + echo "note=skipped: unstructured review" >> "$GITHUB_OUTPUT" + exit 0 + fi + + # The builder exits 3 when there is nothing to judge, so disable + # errexit long enough to read its status instead of aborting on it. + set +e + python3 - "$RUNNER_TEMP/review.json" "$RUNNER_TEMP/adj-prompt.txt" <<'PY' + """Build the adjudicator prompt: every candidate plus the source around it.""" + import json + import os + import sys + + review_path, prompt_path = sys.argv[1], sys.argv[2] + review = json.load(open(review_path)) + candidates = [c for c in (review.get("comments") or []) if c.get("path") and c.get("line")] + if not candidates: + sys.exit(3) # nothing to judge; caller treats this as a clean no-op + + workspace = os.environ.get("GITHUB_WORKSPACE", ".") + CONTEXT_RADIUS = 30 + + def excerpt(path, line): + full = os.path.join(workspace, path) + try: + with open(full, encoding="utf-8", errors="replace") as fh: + lines = fh.read().splitlines() + except OSError: + return "" + lo = max(1, int(line) - CONTEXT_RADIUS) + hi = min(len(lines), int(line) + CONTEXT_RADIUS) + return "\n".join(f"{n:>6} {lines[n - 1]}" for n in range(lo, hi + 1)) + + payload = [] + contexts = [] + for i, c in enumerate(candidates): + cid = f"c{i}" + c["_adj_id"] = cid + payload.append( + { + "id": cid, + "path": c["path"], + "line": c["line"], + "severity": c.get("severity", "suggestion"), + "confidence": c.get("confidence", "medium"), + "body": c.get("body", ""), + } + ) + snippet = excerpt(c["path"], c["line"]) + contexts.append(f"### {cid} -- {c['path']}:{c['line']}\n" + (snippet or "(source unavailable)")) + + guide = "" + guide_rel = os.environ.get("REVIEW_GUIDE_PATH") or "" + if guide_rel: + try: + guide = open(os.path.join(workspace, guide_rel), encoding="utf-8").read() + except OSError: + guide = "" + + instructions = """You are the final code-review adjudicator. Workers produced the candidate + findings below; each carries an `id`. Judge every candidate INDEPENDENTLY and emit exactly one + keep-or-drop verdict per id. Your default for each candidate is keep; drop it only for a + specific reason listed here. There is no target keep count and no quota: candidates do not + compete for slots, and keeping all or dropping all are both correct when the evidence says so. + Never calibrate how many you keep to how many you were shown. Never invent new findings. + + KEEP findings that will produce wrong results, crash, corrupt data, or create a security + exposure under likely conditions. Never drop a concrete crash, data-corruption, or security + finding for lack of a code excerpt. + KEEP findings about wrong, stale, lost, or misattributed data when the candidate gives a + realistic path to it, unless the excerpt proves the finding factually wrong. + KEEP findings that a test provides false coverage: it asserts constants, mocks the system + under test, never exercises the path it claims to, or cannot fail under the regression it is + named to prevent. False coverage is a missing safety net, not test style. + + DROP duplicates, defensive-coding suggestions, style/naming, and nitpicks about test fixtures + or test style -- but a false-coverage finding is never a test-style nitpick. + DROP speculative claims, meaning claims with no stated mechanism ("might break", "could be + missing validation", with no failing input, interleaving, or state named). A candidate that + names a concrete mechanism and its consequence is NOT speculative. + DROP a finding the excerpt proves factually wrong: if it says a case is unhandled or missing + but the excerpt contains the code that already handles it, it is a misread. + DROP a finding that contradicts a decision stated in the project review guide. + Do NOT drop a finding merely because its excerpt does not contain the relevant code. + Cross-file issues and contract violations usually cannot be confirmed from a local excerpt; + judge those on the finding's stated evidence. + + Judge the underlying defect, not the phrasing. For a hedged candidate ("verify that X"), + mentally delete the hedge and test what remains: if a mechanism and a wrong outcome remain, + KEEP it and supply `rewritten_body` asserting that failure directly. If nothing remains but a + request to check, DROP it. + Duplicates are the ONLY comparison between candidates: when several report the same root + cause, keep the strongest and drop the rest as duplicates. + + Respond with ONLY a JSON object between the markers, no prose outside them: + + BEGIN_ADJUDICATION_JSON + {"verdicts": [{"id": "c0", "keep": true, "reason": "...", "rewritten_body": ""}]} + END_ADJUDICATION_JSON + + `rewritten_body` is optional; leave it empty to keep the candidate's own wording. A verdict + never changes a candidate's path or line.""" + instructions = "\n".join(line.strip() for line in instructions.splitlines()) + + parts = [instructions, ""] + if guide.strip(): + parts += ["## Project review guide", "```markdown", guide, "```", ""] + parts += [ + "## Code excerpts around each candidate", + "```", + "\n\n".join(contexts), + "```", + "", + "## Candidates", + "```json", + json.dumps(payload, indent=2), + "```", + ] + open(prompt_path, "w").write("\n".join(parts)) + json.dump(review, open(review_path, "w")) + print(f"Adjudicating {len(payload)} candidate(s).") + PY + build_status=$? + set -e + if [ "$build_status" -eq 3 ]; then + echo "No candidates to adjudicate." + echo "note=no candidates" >> "$GITHUB_OUTPUT" + exit 0 + fi + [ "$build_status" -eq 0 ] || exit "$build_status" + + python3 "$RUNNER_TEMP/bedrock_call.py" \ + "$ADJUDICATOR_MODEL_ID" "$RUNNER_TEMP/adj-prompt.txt" "$RUNNER_TEMP/adj.md" + + python3 - "$RUNNER_TEMP/review.json" "$RUNNER_TEMP/adj.md" <<'PY' + """Apply keep/drop verdicts. Any parse failure exits non-zero so the + step's continue-on-error leaves the unadjudicated review.json in place.""" + import json + import os + import re + import sys + + review_path, adj_path = sys.argv[1], sys.argv[2] + raw = open(adj_path, encoding="utf-8").read() + + block = re.search(r"BEGIN_ADJUDICATION_JSON(.*?)END_ADJUDICATION_JSON", raw, re.S) + text = block.group(1) if block else raw + text = re.sub(r"^\s*```(?:json)?|```\s*$", "", text.strip(), flags=re.M) + verdicts = json.loads(text).get("verdicts") or [] + if not verdicts: + raise ValueError("adjudicator returned no verdicts") + + by_id = {} + for v in verdicts: + vid = str(v.get("id", "")) + if vid: + by_id[vid] = v + + review = json.load(open(review_path)) + candidates = review.get("comments") or [] + kept, dropped = [], [] + for c in candidates: + cid = c.pop("_adj_id", None) + verdict = by_id.get(cid) if cid else None + # No verdict for a candidate means the judge did not rule on it. Keep it: + # an unjudged finding must not be silently lost to a truncated response. + if verdict is not None and verdict.get("keep") is False: + dropped.append((c, str(verdict.get("reason", "")).strip())) + continue + if verdict is not None: + rewritten = str(verdict.get("rewritten_body") or "").strip() + if rewritten: + c["body"] = rewritten + kept.append(c) + + review["comments"] = kept + json.dump(review, open(review_path, "w")) + for c, reason in dropped: + print(f"::notice::Adjudicator dropped {c.get('path')}:{c.get('line')} -- {reason or 'no reason given'}") + print(f"Adjudication kept {len(kept)} of {len(candidates)} candidate(s).") + note_path = os.path.join(os.environ["RUNNER_TEMP"], "adj-note.txt") + with open(note_path, "w") as fh: + fh.write(f"{len(kept)} of {len(candidates)} candidates kept after adjudication") + PY + if [ -f "$RUNNER_TEMP/adj-note.txt" ]; then + echo "note=$(cat "$RUNNER_TEMP/adj-note.txt")" >> "$GITHUB_OUTPUT" + fi + + # --------------------------------------------------------------- + # 8. Reconcile and post. Comment identity is content-based, so a finding + # survives edits that move its line, a human-resolved thread dismisses + # it for good, and the summary is one comment updated in place. # --------------------------------------------------------------- - name: Post Bedrock review id: post_review @@ -391,76 +904,549 @@ jobs: HEAD_SHA: ${{ steps.context.outputs.head_sha }} REVIEWED_SHA: ${{ steps.context.outputs.reviewed_sha }} RUNNER_TEMP_PATH: ${{ runner.temp }} + WORKSPACE_PATH: ${{ github.workspace }} INLINE_MIN_SEVERITY: ${{ inputs.inline_min_severity }} + MIN_CONFIDENCE: ${{ inputs.min_confidence }} MAX_INLINE_COMMENTS: ${{ inputs.max_inline_comments }} SUMMARY_ONLY: ${{ steps.classify.outputs.summary_only }} + HAS_INCREMENTAL: ${{ steps.context.outputs.last_reviewed_sha != '' }} + ADJUDICATION_NOTE: ${{ steps.adjudicate.outputs.note }} with: github-token: ${{ github.token }} script: | - const fs = require("fs"), path = require("path"); + const fs = require("fs"); + const nodePath = require("path"); + const crypto = require("crypto"); + + // ===== BEGIN PURE LOGIC ===== + // Extracted verbatim and unit-tested by tests/reconcile.test.mjs. + // Keep this region free of `github`, `context`, `core`, and all I/O. + + const SEVERITY_RANK = { blocking: 3, suggestion: 2, nitpick: 1 }; + const CONFIDENCE_RANK = { high: 3, medium: 2, low: 1 }; + + // Dismissal matching is narrow and biased toward suppression; live-thread + // matching is wider, because a suppressed variant's claim is still visible + // on a nearby open thread rather than silenced. See FinalWalk ADR-004. + const VARIANT_LINE_WINDOW = 10; + const LIVE_VARIANT_LINE_WINDOW = 40; + const VARIANT_SIMILARITY_THRESHOLD = 0.35; + // A finding counts as fixed only if the code it anchors on actually moved. + const STICKY_LINE_WINDOW = 3; + + const FINGERPRINT_MARKER_RE = //; + // Matches every marker this reviewer writes: the summary's + // `bedrock-reviewer`, an inline `bedrock-review:fp:...`, the + // `bedrock-review:reviewed-sha:...` stamp, and the dismiss note. + const ANY_MARKER_RE = //; + + // Common English plus generic review vocabulary. These words appear in + // nearly every finding body, so they say nothing about WHICH claim is being + // made and would inflate similarity between unrelated findings. + const STOPWORDS = new Set( + ("a an the this that these those it its is are was were be been being has have had do does " + + "did will would can could should may might must shall and or not but if then than so as of " + + "to in on at by for with from into over under when while where which who whom what how why " + + "there here you your we our they their he she his her i me my " + + "code line file function method class value values variable variables field fields " + + "change changes changed issue issues problem error errors case cases " + + "consider could instead please note also use used using add added " + + "check checks checked handle handled missing return returns " + + "review reviewer finding findings comment comments suggestion nitpick blocking").split(" ") + ); + + const BACKTICK_SPAN_RE = /`([^`]+)`/g; + const IDENTIFIER_RE = /\b[A-Za-z_][A-Za-z0-9_]*(?:[.-][A-Za-z_][A-Za-z0-9_]*)+\b|\b[a-z]+(?:_[a-z0-9]+)+\b|\b[a-z]+(?:[A-Z][a-z0-9]*)+\b/g; + const WORD_RE = /[A-Za-z][A-Za-z0-9]*/g; + + function stripMarkers(text) { + return String(text == null ? "" : text).replace(//g, " "); + } + + function normalizeAnchor(text) { + return String(text == null ? "" : text).trim().replace(/\s+/g, " "); + } + + function firstSentence(text) { + const flat = normalizeAnchor(stripMarkers(text)); + const match = flat.match(/^(.+?[.!?])(\s|$)/); + return match ? match[1] : flat.slice(0, 160); + } + + function extractFingerprint(body) { + const match = FINGERPRINT_MARKER_RE.exec(String(body == null ? "" : body)); + return match ? match[1] : ""; + } + + // Content-based identity, stable across line shifts and rewording. Keyed on + // the literal target-line source text, never on model-written prose, so a + // reworded finding keeps its identity. The line NUMBER is deliberately + // excluded. `occurrence` disambiguates files that repeat the same line text, + // and is folded in only when nonzero so the common single-occurrence digest + // stays stable. + function fingerprint(finding) { + const path = String(finding.path == null ? "" : finding.path); + let anchor = normalizeAnchor(finding.lineAnchor); + if (!anchor) anchor = normalizeAnchor(finding.anchor_text); + if (!anchor) anchor = normalizeAnchor(firstSentence(finding.body)); + const occurrence = Number(finding.occurrence || 0); + const keyed = occurrence > 0 + ? path + "\u0000" + anchor + "\u0000#" + occurrence + : path + "\u0000" + anchor; + return crypto.createHash("sha256").update(keyed).digest("hex").slice(0, 12); + } + + function normalizeWord(word) { + const lower = word.toLowerCase(); + if (lower.length > 3 && lower.endsWith("s") && !lower.endsWith("ss")) return lower.slice(0, -1); + return lower; + } + + // The distinctive vocabulary of a claim. Identifiers and backticked code + // spans dominate: they survive rewording, so two phrasings of one claim + // still share tokens. Multi-word backtick spans are mined for identifiers + // rather than kept whole, since their exact text varies between rewordings. + function claimTokens(text) { + const tokens = new Set(); + const clean = stripMarkers(text); + let span; + BACKTICK_SPAN_RE.lastIndex = 0; + while ((span = BACKTICK_SPAN_RE.exec(clean)) !== null) { + const inner = span[1].trim(); + // Stopwords stay stopwords inside backticks: reviewers backtick common + // words (`error`, `None`), and letting those through would let two + // unrelated findings share tokens purely on formatting. + if (inner && !inner.includes(" ") && !STOPWORDS.has(inner.toLowerCase())) { + tokens.add(inner.toLowerCase()); + } + } + for (const ident of clean.match(IDENTIFIER_RE) || []) tokens.add(ident.toLowerCase()); + for (const word of clean.match(WORD_RE) || []) { + const normalized = normalizeWord(word); + if (!STOPWORDS.has(normalized) && !STOPWORDS.has(word.toLowerCase())) tokens.add(normalized); + } + return tokens; + } + + function jaccard(a, b) { + if (!a.size || !b.size) return 0; + let shared = 0; + for (const token of a) if (b.has(token)) shared++; + return shared / (a.size + b.size - shared); + } + + // Highest-similarity record that qualifies as a reworded variant of the + // candidate, or null. A candidate with no real line anchor cannot establish + // locality, so it gets exact-fingerprint matching only. + function bestVariantMatch(candidate, records, window) { + const line = Number(candidate.line || 0); + if (line <= 0) return null; + const tokens = claimTokens(candidate.body); + let best = null; + for (const record of records) { + if (record.path !== candidate.path) continue; + const recordLine = Number(record.line || 0); + if (recordLine <= 0) continue; + if (Math.abs(recordLine - line) > window) continue; + const score = jaccard(tokens, record.tokens); + if (score >= VARIANT_SIMILARITY_THRESHOLD && (!best || score > best.score)) { + best = { record: record, score: score }; + } + } + return best; + } + + // A missing or unknown grade is treated as the floor itself, so an omitted + // field never silently drops a finding. + function meetsFloor(table, value, floor) { + const floorRank = table[floor] === undefined ? Math.min.apply(null, Object.values(table)) : table[floor]; + const rank = table[String(value == null ? "" : value).toLowerCase()]; + return (rank === undefined ? floorRank : rank) >= floorRank; + } + + // Did the code this comment anchors on change since the last reviewed + // commit? This is what separates a genuinely fixed finding from one the + // model simply did not re-emit on this run. + function lineChanged(ranges, path, line, window) { + const spans = ranges[path]; + if (!spans) return false; + const lo = Number(line) - window; + const hi = Number(line) + window; + return spans.some(function (span) { return span[0] <= hi && span[1] >= lo; }); + } + + // Which existing threads does this run still assert? A thread survives if + // any finding that got past dismissal carries its fingerprint, whichever + // presentation bucket that finding landed in (inline thread, minor note, + // non-diff note, or dropped by a confidence floor), or if a candidate was + // reconciled into it as a reworded variant. Only a claim the model did not + // restate at all is a deletion candidate, and even then only when the code + // it anchors on changed. A finding routed elsewhere, or graded differently, + // is not evidence that its bug was fixed. + function assertedFingerprints(findings, existingFps, variantMatchedFps) { + const asserted = new Set(variantMatchedFps || []); + for (const finding of findings) { + if (finding.fp && existingFps.has(finding.fp)) asserted.add(finding.fp); + } + return asserted; + } + + // The ordinal position of `line` among all lines in the file carrying the + // same normalized text. Not a line number: adding or removing unrelated + // lines elsewhere does not change which occurrence a site is. + function anchorOccurrence(lines, line) { + const target = normalizeAnchor(lines[line - 1]); + if (!target) return 0; + let seen = 0; + for (let i = 0; i < lines.length; i++) { + if (normalizeAnchor(lines[i]) !== target) continue; + if (i === line - 1) return seen; + seen++; + } + return 0; + } + // ===== END PURE LOGIC ===== + const tmp = process.env.RUNNER_TEMP_PATH; + const workspace = process.env.WORKSPACE_PATH; const pull_number = Number(process.env.PR_NUMBER); const commit_id = process.env.HEAD_SHA; - const sha = (process.env.REVIEWED_SHA || "").trim(); + const shortSha = (process.env.REVIEWED_SHA || "").trim(); + const headSha = (process.env.HEAD_SHA || "").trim(); const { owner, repo } = context.repo; - const marker = ""; - const mode = fs.readFileSync(path.join(tmp, "review-mode.txt"), "utf8").trim(); + const SUMMARY_MARKER = ""; + const BOT_LOGINS = new Set(["github-actions[bot]", "github-actions"]); + const isBot = (c) => BOT_LOGINS.has(c.user && c.user.login) || (c.user && c.user.type === "Bot"); + const isOurs = (c) => isBot(c) && ANY_MARKER_RE.test(c.body || ""); + + // One persistent summary comment, updated in place. Earlier versions of this + // workflow posted the summary as a PR review instead; those stay as + // historical records and are never touched. + async function upsertSummary(body) { + const stamped = headSha + ? body + "\n\n" + : body; + const existing = await github.paginate(github.rest.issues.listComments, + { owner, repo, issue_number: pull_number, per_page: 100 }); + const ours = existing.filter((c) => isBot(c) && (c.body || "").includes(SUMMARY_MARKER)); + if (ours.length) { + for (const extra of ours.slice(1)) { + try { + await github.rest.issues.deleteComment({ owner, repo, comment_id: extra.id }); + } catch (e) { core.warning("delete duplicate summary " + extra.id + ": " + e.message); } + } + const updated = await github.rest.issues.updateComment( + { owner, repo, comment_id: ours[0].id, body: stamped }); + return updated.data.html_url; + } + const created = await github.rest.issues.createComment( + { owner, repo, issue_number: pull_number, body: stamped }); + return created.data.html_url; + } + const mode = fs.readFileSync(nodePath.join(tmp, "review-mode.txt"), "utf8").trim(); if (mode !== "JSON") { - let body = fs.readFileSync(path.join(tmp, "review-fallback.md"), "utf8").trim(); - body = `${marker}\n_Bedrock review of \`${sha}\`_\n\n${body}`; - const c = await github.rest.issues.createComment({ owner, repo, issue_number: pull_number, body }); - core.setOutput("review_url", c.data.html_url); return; + const raw = fs.readFileSync(nodePath.join(tmp, "review-fallback.md"), "utf8").trim(); + let body = SUMMARY_MARKER + "\n_Bedrock review of `" + shortSha + "` (unstructured output)_\n\n" + raw; + if (body.length > 65000) body = body.slice(0, 64950) + "\n\n_[truncated]_"; + core.setOutput("review_url", await upsertSummary(body)); + return; } - const review = JSON.parse(fs.readFileSync(path.join(tmp, "review.json"), "utf8")); + const review = JSON.parse(fs.readFileSync(nodePath.join(tmp, "review.json"), "utf8")); const rawComments = Array.isArray(review.comments) ? review.comments : []; - let botLocs = new Set(); + let changedRanges = {}; + try { + changedRanges = JSON.parse(fs.readFileSync(nodePath.join(tmp, "changed-ranges.json"), "utf8")); + } catch (e) { core.warning("changed ranges unavailable: " + e.message); } + const hasIncremental = process.env.HAS_INCREMENTAL === "true"; + + // Resolve each candidate's identity from the checked-out source, not from + // anything the model wrote. + const fileCache = new Map(); + function fileLines(path) { + if (!fileCache.has(path)) { + let lines = null; + try { + lines = fs.readFileSync(nodePath.join(workspace, path), "utf8").split("\n"); + } catch (e) { lines = null; } + fileCache.set(path, lines); + } + return fileCache.get(path); + } + + const candidates = []; + for (const c of rawComments) { + if (!c.path || !c.line || !c.body) continue; + const lines = fileLines(c.path); + const lineAnchor = lines && lines[c.line - 1] !== undefined ? lines[c.line - 1] : ""; + const occurrence = lines && lineAnchor ? anchorOccurrence(lines, c.line) : 0; + candidates.push(Object.assign({}, c, { + lineAnchor: lineAnchor, + occurrence: occurrence, + fp: fingerprint({ + path: c.path, lineAnchor: lineAnchor, anchor_text: c.anchor_text, + body: c.body, occurrence: occurrence, + }), + })); + } + + // Existing inline comments this reviewer owns, plus their thread state. + let ourComments = []; + try { + const existing = await github.paginate(github.rest.pulls.listReviewComments, + { owner, repo, pull_number, per_page: 100 }); + ourComments = existing.filter(isOurs); + } catch (e) { core.warning("existing comments: " + e.message); } + + // Thread resolution is the dismissal signal, and a human reply is a + // do-not-destroy signal. Both come from one GraphQL query. + const resolvedIds = new Set(); + const humanRepliedIds = new Set(); try { - const existing = await github.paginate(github.rest.pulls.listReviewComments, { owner, repo, pull_number, per_page: 100 }); - for (const c of existing) { - if (c.user.type === "Bot" || c.user.login === "github-actions[bot]") { - const line = c.original_line ?? c.line; - if (c.path && line) botLocs.add(`${c.path}:${line}`); + let cursor = null; + for (let page = 0; page < 20; page++) { + const result = await github.graphql( + `query($owner: String!, $repo: String!, $pr: Int!, $cursor: String) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $pr) { + reviewThreads(first: 100, after: $cursor) { + pageInfo { hasNextPage endCursor } + nodes { + isResolved + comments(first: 50) { + nodes { databaseId author { login } } + } + } + } + } + } + }`, + { owner, repo, pr: pull_number, cursor }); + const threads = result.repository.pullRequest.reviewThreads; + for (const thread of threads.nodes) { + const ids = thread.comments.nodes.map((n) => n.databaseId).filter(Boolean); + const humanReplied = thread.comments.nodes.some( + (n) => n.author && !BOT_LOGINS.has(n.author.login) && !/\[bot\]$/.test(n.author.login)); + for (const id of ids) { + if (thread.isResolved) resolvedIds.add(id); + if (humanReplied) humanRepliedIds.add(id); + } } + if (!threads.pageInfo.hasNextPage) break; + cursor = threads.pageInfo.endCursor; } - } catch (e) { core.warning(`existing comments: ${e.message}`); } + } catch (e) { + // Fail open: without thread state, treat every thread as unresolved and + // human-replied, which suppresses nothing and destroys nothing. + core.warning("review threads unavailable, keeping all existing threads: " + e.message); + for (const c of ourComments) humanRepliedIds.add(c.id); + } - const { data: prFiles } = await github.rest.pulls.listFiles({ owner, repo, pull_number }); - const commentable = {}; - for (const f of prFiles) { - commentable[f.filename] = new Set(); - if (!f.patch) continue; - let n = 0; - for (const l of f.patch.split("\n")) { - if (l.startsWith("@@")) { const m = l.match(/@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/); if (m) n = parseInt(m[1],10)-1; } - else if (l.startsWith("-")) {} else { n++; commentable[f.filename].add(n); } + const toRecord = (c) => ({ + path: c.path, + line: c.original_line == null ? c.line : c.original_line, + fp: extractFingerprint(c.body), + tokens: claimTokens(c.body), + id: c.id, + body: c.body, + }); + const dismissed = ourComments.filter((c) => resolvedIds.has(c.id)).map(toRecord); + const liveComments = ourComments.filter((c) => !resolvedIds.has(c.id)); + const liveRecords = liveComments.map(toRecord); + + const existingByFp = new Map(); + for (const c of liveComments) { + const fp = extractFingerprint(c.body); + if (fp && !existingByFp.has(fp)) existingByFp.set(fp, c); + } + + // A human ruled on the dismissed claim, so wrongly suppressing a nearby new + // finding costs less than re-raising a dismissed one. + const dismissedFps = new Set(dismissed.map((d) => d.fp).filter(Boolean)); + const surviving = []; + let suppressed = 0; + for (const c of candidates) { + if (dismissedFps.has(c.fp)) { + suppressed++; + core.info("Suppressed dismissed finding at " + c.path + ":" + c.line); + continue; } + const variant = bestVariantMatch(c, dismissed, VARIANT_LINE_WINDOW); + if (variant) { + suppressed++; + core.notice("Suppressed reworded variant of a dismissed finding: " + c.path + ":" + c.line + + " matches dismissal at " + variant.record.path + ":" + variant.record.line + + " (similarity " + variant.score.toFixed(2) + ")"); + continue; + } + surviving.push(c); } - const SEV = { blocking: 3, suggestion: 2, nitpick: 1 }; - const floor = SEV[String(process.env.INLINE_MIN_SEVERITY||"suggestion").toLowerCase()] ?? SEV.suggestion; - const pm = parseInt(process.env.MAX_INLINE_COMMENTS, 10); - const maxInline = Number.isInteger(pm) && pm >= 0 ? pm : 10; + // Which lines can actually carry an inline comment on this diff. + const commentable = {}; + try { + const prFiles = await github.paginate(github.rest.pulls.listFiles, + { owner, repo, pull_number, per_page: 100 }); + for (const f of prFiles) { + commentable[f.filename] = new Set(); + if (!f.patch) continue; + let n = 0; + for (const l of f.patch.split("\n")) { + if (l.startsWith("@@")) { + const m = l.match(/@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/); + if (m) n = parseInt(m[1], 10) - 1; + } else if (l.startsWith("-")) { + // removed line: no right-hand position + } else { n++; commentable[f.filename].add(n); } + } + } + } catch (e) { core.warning("pr files: " + e.message); } + + const severityFloor = String(process.env.INLINE_MIN_SEVERITY || "suggestion").toLowerCase(); + const confidenceFloor = String(process.env.MIN_CONFIDENCE || "medium").toLowerCase(); + const parsedMax = parseInt(process.env.MAX_INLINE_COMMENTS, 10); + const maxInline = Number.isInteger(parsedMax) && parsedMax >= 0 ? parsedMax : 10; const summaryOnly = process.env.SUMMARY_ONLY === "true"; - const cand = [], minor = [], nonDiff = []; let dropped = 0; - for (const c of rawComments) { - if (!c.path || !c.line || !c.body) continue; - if (botLocs.has(`${c.path}:${c.line}`)) { dropped++; continue; } - const sev = String(c.severity||"suggestion").toLowerCase(); - const rank = SEV[sev] ?? SEV.suggestion; - if (summaryOnly || rank < floor) { minor.push(`**\`${c.path}:${c.line}\`** (${sev}): ${c.body}`); continue; } - if (commentable[c.path]?.has(c.line)) cand.push({ path: c.path, line: c.line, side: "RIGHT", body: c.body, rank }); - else nonDiff.push(`**\`${c.path}:${c.line}\`**: ${c.body}`); + const inlineCandidates = []; + const reported = []; + const minor = []; + const nonDiff = []; + let lowConfidence = 0; + for (const c of surviving) { + if (!meetsFloor(CONFIDENCE_RANK, c.confidence, confidenceFloor)) { + lowConfidence++; + core.info("Dropped below the " + confidenceFloor + " confidence floor: " + c.path + ":" + c.line); + continue; + } + reported.push(c); + const severity = String(c.severity || "suggestion").toLowerCase(); + if (summaryOnly || !meetsFloor(SEVERITY_RANK, severity, severityFloor)) { + minor.push("**`" + c.path + ":" + c.line + "`** (" + severity + "): " + c.body); + continue; + } + if (commentable[c.path] && commentable[c.path].has(c.line)) { + inlineCandidates.push(Object.assign({}, c, { rank: SEVERITY_RANK[severity] || 2 })); + } else { + nonDiff.push("**`" + c.path + ":" + c.line + "`**: " + c.body); + } + } + + // A reworded restatement of a claim already open on a nearby thread + // reconciles with that thread instead of duplicating it. + const openFps = new Set(liveRecords.map((r) => r.fp).filter(Boolean)); + const variantMatchedFps = new Set(); + const deduped = []; + for (const c of inlineCandidates) { + if (openFps.has(c.fp)) { deduped.push(c); continue; } + const variant = bestVariantMatch(c, liveRecords, LIVE_VARIANT_LINE_WINDOW); + if (variant) { + // The thread this reconciles into must be marked as still asserted, + // or the orphan sweep below would delete it on this same run and the + // claim would vanish along with the candidate we just dropped. + if (variant.record.fp) variantMatchedFps.add(variant.record.fp); + core.notice("Reconciled reworded finding with the open thread at " + + variant.record.path + ":" + variant.record.line + " (similarity " + + variant.score.toFixed(2) + "); not posting a duplicate"); + continue; + } + deduped.push(c); } - cand.sort((a,b) => b.rank-a.rank || a.path.localeCompare(b.path) || a.line-b.line); - const inline = cand.slice(0, maxInline).map(({path,line,side,body}) => ({path,line,side,body})); - const overflow = cand.slice(maxInline).map(c => `**\`${c.path}:${c.line}\`**: ${c.body}`); - // Copilot-style header: an explicit verdict with finding counts, so a - // clean run reads as a clear "no issues found" review, not silence. + // The cap limits how many NEW threads one run opens. A finding that already + // has a live thread is always reconciled: dropping it here because a + // higher-severity finding appeared would delete a thread whose bug is still + // present, which is the churn this reconcile exists to prevent. + const reEmitted = deduped.filter((c) => existingByFp.has(c.fp)); + const brandNew = deduped.filter((c) => !existingByFp.has(c.fp)); + brandNew.sort((a, b) => b.rank - a.rank || a.path.localeCompare(b.path) || a.line - b.line); + const keep = reEmitted.concat(brandNew.slice(0, maxInline)); + const overflow = brandNew.slice(maxInline).map((c) => "**`" + c.path + ":" + c.line + "`**: " + c.body); + + // Reconcile. Rules, in order (FinalWalk ADR-004): + // 1. Re-emitted at the same line: no-op, or patch the body in place. + // 2. Re-emitted at a moved line: delete and repost, unless a human + // replied on the thread, in which case leave it (deleting would + // destroy the reply). + // 3. Not re-emitted: delete as fixed ONLY when the anchored code changed + // since the last reviewed commit. On unchanged code, non-reemission is + // sampling noise; the code cannot have been fixed by not changing. + // 4. In a resolved thread: never touched. + const stats = { posted: 0, moved: 0, patched: 0, unchanged: 0, deleted: 0, sticky: 0, failed: 0 }; + const matchedFps = assertedFingerprints( + surviving, new Set(existingByFp.keys()), variantMatchedFps); + + for (const c of keep) { + const marker = "\n\n"; + const body = c.body.trim() + "\n\nconfidence: " + + String(c.confidence || "medium").toLowerCase() + "" + marker; + const existing = existingByFp.get(c.fp); + if (existing) { + matchedFps.add(c.fp); + const existingLine = existing.line == null ? existing.original_line : existing.line; + if (existingLine === c.line) { + if ((existing.body || "").trim() !== body.trim()) { + try { + await github.rest.pulls.updateReviewComment( + { owner, repo, comment_id: existing.id, body }); + stats.patched++; + } catch (e) { core.warning("patch " + existing.id + ": " + e.message); stats.failed++; } + } else { stats.unchanged++; } + continue; + } + if (humanRepliedIds.has(existing.id)) { + core.notice("Finding moved to " + c.path + ":" + c.line + " but its thread has a human reply; " + + "leaving the existing comment in place"); + stats.sticky++; + continue; + } + // The REST API cannot relocate an inline comment, so a move is a + // delete plus a fresh post. Post first: if the post fails, the + // original comment must survive rather than the finding vanishing. + try { + await github.rest.pulls.createReviewComment( + { owner, repo, pull_number, commit_id, path: c.path, line: c.line, side: "RIGHT", body }); + try { + await github.rest.pulls.deleteReviewComment({ owner, repo, comment_id: existing.id }); + } catch (e) { core.warning("delete stale " + existing.id + ": " + e.message); } + stats.moved++; + } catch (e) { core.warning("repost " + c.path + ":" + c.line + ": " + e.message); stats.failed++; } + continue; + } + try { + await github.rest.pulls.createReviewComment( + { owner, repo, pull_number, commit_id, path: c.path, line: c.line, side: "RIGHT", body }); + stats.posted++; + } catch (e) { + core.warning("post " + c.path + ":" + c.line + ": " + e.message); + nonDiff.push("**`" + c.path + ":" + c.line + "`**: " + c.body); + stats.failed++; + } + } + + for (const [fp, existing] of existingByFp) { + if (matchedFps.has(fp)) continue; + if (humanRepliedIds.has(existing.id)) { stats.sticky++; continue; } + const line = existing.line == null ? existing.original_line : existing.line; + // No incremental data at all (first review, force-push, unreachable prior + // SHA) means there is no way to tell changed from unchanged, so keep the + // comment rather than guessing it was fixed. + if (!hasIncremental || !lineChanged(changedRanges, existing.path, line, STICKY_LINE_WINDOW)) { + core.notice("Keeping " + existing.path + ":" + line + + " -- not re-emitted, but the code it anchors on did not change"); + stats.sticky++; + continue; + } + try { + await github.rest.pulls.deleteReviewComment({ owner, repo, comment_id: existing.id }); + stats.deleted++; + } catch (e) { core.warning("delete fixed " + existing.id + ": " + e.message); } + } + + // Verdict header: an explicit finding count, so a clean run reads as a + // clear "no issues found" rather than as silence. const VERDICT_LABEL = { APPROVE: "No issues found", REQUEST_CHANGES: "Changes suggested", @@ -474,46 +1460,82 @@ jobs: const verdictKey = VERDICT_ALIASES[rawVerdict] || rawVerdict; let verdictNote = VERDICT_LABEL[verdictKey] || VERDICT_LABEL.COMMENT; const counts = { blocking: 0, suggestion: 0, nitpick: 0 }; - for (const c of rawComments) { + for (const c of reported) { const s = String(c.severity || "suggestion").toLowerCase(); if (counts[s] !== undefined) counts[s]++; } - const plural = (n, w) => `${n} ${w}${n === 1 ? "" : "s"}`; + const plural = (n, w) => n + " " + w + (n === 1 ? "" : "s"); const parts = []; if (counts.blocking) parts.push(plural(counts.blocking, "issue")); if (counts.suggestion) parts.push(plural(counts.suggestion, "suggestion")); if (counts.nitpick) parts.push(plural(counts.nitpick, "nitpick")); - if (!parts.length && !rawComments.length) verdictNote = VERDICT_LABEL.APPROVE; + if (!parts.length && !reported.length) verdictNote = VERDICT_LABEL.APPROVE; const findingsLine = parts.length ? parts.join(", ") : "no findings"; - let body = `${marker}`; - body += `\n## Pull request overview`; - if (sha) body += `\n_Bedrock review of \`${sha}\`_`; - body += `\n\n**Verdict:** ${verdictNote}, ${findingsLine} _(advisory; never blocks merge)_`; - body += `\n\n${(review.summary||"").trim()}`; - const sec = (t, a) => a.length ? `\n\n---\n\n**${t}:**\n` + a.map(x => `- ${x}`).join("\n") : ""; - body += sec("Minor notes", minor) + sec("Findings on non-diff lines", nonDiff) + sec(`Additional findings (over the inline limit of ${maxInline})`, overflow); - if (body.length > 65000) body = body.slice(0, 64950) + "\n\n_[truncated]_"; + let body = SUMMARY_MARKER; + body += "\n## Pull request overview"; + if (shortSha) body += "\n_Bedrock review of `" + shortSha + "`_"; + body += "\n\n**Verdict:** " + verdictNote + ", " + findingsLine + " _(advisory; never blocks merge)_"; + body += "\n\n" + String(review.summary || "").trim(); + + const section = (title, items) => items.length + ? "\n\n---\n\n**" + title + ":**\n" + items.map((x) => "- " + x).join("\n") + : ""; + body += section("Minor notes", minor); + body += section("Findings on non-diff lines", nonDiff); + body += section("Additional findings (over the inline limit of " + maxInline + ")", overflow); - const { data: posted } = await github.rest.pulls.createReview({ owner, repo, pull_number, commit_id, event: "COMMENT", body, comments: inline }); - core.setOutput("review_url", posted.html_url); + const provenance = []; + const adjNote = String(process.env.ADJUDICATION_NOTE || "").trim(); + if (adjNote) provenance.push(adjNote); + if (lowConfidence) provenance.push(lowConfidence + " below the " + confidenceFloor + " confidence floor"); + if (suppressed) provenance.push(suppressed + " previously dismissed"); + provenance.push("threads: " + stats.posted + " new, " + stats.moved + " moved, " + + stats.unchanged + " unchanged, " + stats.patched + " updated, " + + stats.deleted + " resolved as fixed, " + stats.sticky + " kept"); + body += "\n\n" + provenance.join(" · ") + ""; + + if (body.length > 65000) body = body.slice(0, 64950) + "\n\n_[truncated]_"; + core.setOutput("review_url", await upsertSummary(body)); + core.info("Reconcile stats: " + JSON.stringify(stats)); # --------------------------------------------------------------- - # 8. Job summary. + # 9. Job summary. # --------------------------------------------------------------- - name: Write run summary if: always() shell: bash env: + COMMAND: ${{ steps.trigger.outputs.command }} REVIEW_URL: ${{ steps.post_review.outputs.review_url }} BEDROCK_OUTCOME: ${{ steps.bedrock.outcome }} + ADJUDICATE_OUTCOME: ${{ steps.adjudicate.outcome }} + ADJUDICATION_NOTE: ${{ steps.adjudicate.outputs.note }} PROCEED: ${{ steps.gate.outputs.proceed }} SKIP_REASON: ${{ steps.gate.outputs.reason }} run: | { echo "## Bedrock PR Review" echo - if [ "${PROCEED:-true}" != "true" ]; then echo "Skipped: ${SKIP_REASON:-not eligible}."; - elif [ "$BEDROCK_OUTCOME" = "success" ] && [ -n "${REVIEW_URL:-}" ]; then echo "Review posted: $REVIEW_URL"; - else echo ":information_source: Review skipped this run (advisory, non-blocking). Check the Bedrock step logs."; fi + case "${COMMAND:-review}" in + ignore) + echo "No action: the comment did not contain the trigger phrase." ;; + dismiss) + echo "Dismissed every finding this reviewer posted." ;; + *) + if [ "${PROCEED:-true}" != "true" ]; then + echo "Skipped: ${SKIP_REASON:-not eligible}." + elif [ "$BEDROCK_OUTCOME" = "success" ] && [ -n "${REVIEW_URL:-}" ]; then + echo "Review posted: $REVIEW_URL" + if [ "${ADJUDICATE_OUTCOME:-skipped}" = "failure" ]; then + echo + echo ":warning: Adjudication failed; every candidate finding was kept unjudged." + elif [ -n "${ADJUDICATION_NOTE:-}" ]; then + echo + echo "Adjudication: ${ADJUDICATION_NOTE}" + fi + else + echo ":information_source: Review skipped this run (advisory, non-blocking). Check the Bedrock step logs." + fi ;; + esac } >> "$GITHUB_STEP_SUMMARY" diff --git a/.github/workflows/check-prompt-embed.yml b/.github/workflows/check-prompt-embed.yml index 1930d8c..4deeace 100644 --- a/.github/workflows/check-prompt-embed.yml +++ b/.github/workflows/check-prompt-embed.yml @@ -1,8 +1,9 @@ # Guards against prompt drift. The review prompt lives in one file # (.github/codex/prompts/codex-pr-review.md) but is embedded as base64 -# (PROMPT_B64) in codex-pr-review.yml so consumers on internal repos can run -# the reviewer without a cross-repo checkout. This job fails if the embedded -# copy no longer matches the file, so the two can never silently diverge. +# (PROMPT_B64) in every reviewer workflow that needs it, so consumers on +# internal repos can run the reviewer without a cross-repo checkout. This job +# fails if any embedded copy no longer matches the file, so they can never +# silently diverge. # # To fix a failure, regenerate the blob and paste it as PROMPT_B64: # base64 < .github/codex/prompts/codex-pr-review.md | tr -d '\n' @@ -14,12 +15,14 @@ on: paths: - .github/codex/prompts/codex-pr-review.md - .github/workflows/codex-pr-review.yml + - .github/workflows/bedrock-pr-review.yml - .github/workflows/check-prompt-embed.yml push: branches: [main] paths: - .github/codex/prompts/codex-pr-review.md - .github/workflows/codex-pr-review.yml + - .github/workflows/bedrock-pr-review.yml permissions: contents: read @@ -35,18 +38,26 @@ jobs: run: | set -euo pipefail prompt=.github/codex/prompts/codex-pr-review.md - workflow=.github/workflows/codex-pr-review.yml - expected="$(base64 < "$prompt" | tr -d '\n')" - embedded="$(grep -oE 'PROMPT_B64: "[^"]+"' "$workflow" | sed -E 's/PROMPT_B64: "(.*)"/\1/')" + status=0 + + for workflow in \ + .github/workflows/codex-pr-review.yml \ + .github/workflows/bedrock-pr-review.yml + do + embedded="$(grep -oE 'PROMPT_B64: "[^"]+"' "$workflow" | sed -E 's/PROMPT_B64: "(.*)"/\1/')" + if [ -z "$embedded" ]; then + echo "::error::PROMPT_B64 not found in $workflow." >&2 + status=1 + continue + fi + if [ "$expected" != "$embedded" ]; then + echo "::error::PROMPT_B64 in $workflow is out of sync with $prompt." >&2 + echo "Regenerate: base64 < $prompt | tr -d '\\n'" >&2 + status=1 + continue + fi + echo "$workflow: embedded prompt is in sync with $prompt." + done - if [ -z "$embedded" ]; then - echo "::error::PROMPT_B64 not found in $workflow." >&2 - exit 1 - fi - if [ "$expected" != "$embedded" ]; then - echo "::error::PROMPT_B64 in $workflow is out of sync with $prompt." >&2 - echo "Regenerate: base64 < $prompt | tr -d '\\n'" >&2 - exit 1 - fi - echo "Embedded prompt is in sync with $prompt." + exit "$status" diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml new file mode 100644 index 0000000..01c5abe --- /dev/null +++ b/.github/workflows/tests.yml @@ -0,0 +1,35 @@ +name: Tests + +# Validates this repo's own reusable workflows. The reconcile logic that decides +# which review comments to post, move, keep, or delete lives inside +# bedrock-pr-review.yml; tests/reconcile.test.mjs extracts that region from the +# YAML and exercises it, so the workflow file is the only copy. + +on: + pull_request: + push: + branches: [main] + +permissions: + contents: read + +jobs: + test: + name: Workflow tests + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-node@v4 + with: + node-version: "20" + + - name: Reconcile logic tests + run: node --test tests/ + + # Every workflow here must be loadable as YAML and every embedded script + # must parse. A raw control character or an unbalanced brace inside a + # block scalar otherwise fails at dispatch time in a consumer repo, where + # it is far more expensive to notice. + - name: Validate embedded scripts + run: python3 scripts/validate-workflows.py diff --git a/README.md b/README.md index fc4e8f3..7429496 100644 --- a/README.md +++ b/README.md @@ -243,6 +243,119 @@ action, and no review. --- +## `bedrock-pr-review`: AI code review through your own AWS account + +The same review shape as `codex-pr-review`, but the model call goes to AWS Bedrock and +authentication is GitHub OIDC into an IAM role, so there is no API token to store or +rotate and no code leaves your AWS boundary. Advisory and non-blocking, like the Codex +reviewer. + +Its review lifecycle follows the FinalWalk standard +([`digital-analytics-len/FinalWalk`](https://github.com/digital-analytics-len/FinalWalk), +ADR-004), because positional comment identity is what makes an AI reviewer noisy: + +- **Findings survive edits that move them.** A comment's identity is the literal source + text of the line it sits on, hashed, not the line number. An unrelated change earlier + in the file no longer deletes and re-posts every finding below it, so an + acknowledgement or a resolution survives the next push. +- **Resolving a thread dismisses that finding for good.** Resolution is the dismissal + signal. The exact finding is suppressed on every later run, and so is a nearby + reworded restatement of it (same file, within 10 lines, claim-token similarity + ≥ 0.35). Each suppression prints a `::notice::` in the run log, so nothing is dropped + silently. +- **Re-reviews converge instead of treadmilling.** A finding the model does not re-emit + is deleted as fixed only when the code it anchors on actually changed since the last + reviewed commit. On unchanged code, non-reemission is sampling noise: the code cannot + have been fixed by not changing. A restatement of a claim already open on a nearby + thread reconciles with that thread rather than posting a duplicate. +- **A thread a human replied to is never destroyed.** Not on a move, not on a rescan. +- **One summary comment, updated in place.** Instead of a new review per push. It stamps + the commit it reviewed, which is also how the next run knows what to skip. +- **Every finding is rechecked before it posts.** An adjudication pass sends each + candidate back with the surrounding source and drops the ones that source refutes + (a "missing check" whose excerpt contains the check), plus duplicates, speculation + with no named mechanism, and anything contradicting your review guide. It is + fail-open: an adjudicator error keeps the full candidate set rather than losing + findings. + +### Caller + +Copy [`examples/pr-review-bedrock.yml`](examples/pr-review-bedrock.yml) to +`.github/workflows/pr-review.yml` and set `aws_role_arn`. The role must allow +`bedrock:InvokeModel` / `bedrock:Converse` on the model and trust your repo via OIDC. +No secrets are needed. + +Keeping the `issue_comment` and `pull_request_review_comment` triggers enables: + +| Comment | Effect | +|---|---| +| `@bedrock-review` | Re-review the current HEAD, even if it was already reviewed or the PR is a draft. | +| `@bedrock-review dismiss` | Delete every finding this reviewer posted. Resolved threads are left as the permanent record. | + +### Make it sharper per repo + +Drop a `.github/review-guide.md` with your project's actual blocker and architecture +rules. It is fed to both passes, and the adjudicator drops any finding that contradicts +it. Concrete rules that point at real defects work; style preferences do not. + +```markdown +# Review guide + +## Blocker rules +- No user-controlled input may reach shell execution or dynamic eval. +- Auth checks must fail closed when user, role, tenant, or scope is missing. + +## Architecture rules +- Falcon aggregates and reshapes; business rules belong in TIM or ItemLib. +- API handlers call services, not repositories directly. +``` + +### Inputs + +| Input | Required | Default | Description | +|---|---|---|---| +| `aws_role_arn` | **Yes** | n/a | IAM role assumed via GitHub OIDC. | +| `aws_region` | No | `us-east-1` | Bedrock region. | +| `model_id` | No | `openai.gpt-5.6-terra` | Finding model. `openai.*` tiers use the Responses API on bedrock-mantle; everything else uses Converse. | +| `adjudicator_model_id` | No | `us.anthropic.claude-sonnet-4-6-v1:0` | Model for the verification pass. Precision is model-insensitive once verification is on, but the judgment's recall is not: a weak judge silently drops real hedged findings, so this stays strong regardless of the finding model. | +| `adjudicate` | No | `true` | Run the verification pass. | +| `min_confidence` | No | `medium` | Drop findings below this self-reported confidence (`low`/`medium`/`high`). A finding with no confidence field counts as `medium`, so an omitted field never silently drops it. | +| `inline_min_severity` | No | `suggestion` | Minimum severity that opens an inline thread. Below the floor goes under "Minor notes". | +| `max_inline_comments` | No | `10` | Cap on inline threads per run; overflow is listed in the body. | +| `max_files` | No | `50` | Skip PRs touching more files than this; `0` disables. A PR far over the cap blows the diff budget and yields shallow findings. | +| `review_guide_path` | No | `.github/review-guide.md` | Project rules, fed to both passes. A missing file is not an error. | +| `trigger_phrase` | No | `@bedrock-review` | On-demand review phrase. Must match the phrase in the caller's `if`. | +| `pr_number` | No | auto | Resolved from the triggering event; pass it only for `workflow_dispatch`. | +| `max_tokens` | No | `4096` | Max tokens per model response. | +| `debounce_seconds` | No | `30` | Collapse a burst of pushes into one review. Never applied to an on-demand request. | + +Dependabot PRs are skipped: they run without access to repo or org secrets, so the role +assumption cannot succeed, and a no-op beats a red check. + +### Outputs + +| Output | Description | +|---|---| +| `review_url` | URL of the persistent review summary comment. | + +### Tests + +The reconcile logic (which comments to post, move, keep, or delete) is unit-tested. +`tests/reconcile.test.mjs` extracts the workflow's `PURE LOGIC` region from the YAML at +test time and runs it, so there is no second copy to drift out of sync: + +```bash +node --test tests/ +python3 scripts/validate-workflows.py +``` + +`validate-workflows.py` loads every workflow here as YAML and parses each embedded +bash, Python, and JavaScript block. A reusable workflow only fails when a consumer +dispatches it, so a syntax error in a block scalar is otherwise invisible until it +breaks somebody else's PR. + +--- + ## `secret-scan`: block secrets and flag PII on every PR A CI-side gate that stops hardcoded secrets (API keys, tokens, private keys, diff --git a/examples/pr-review-bedrock.yml b/examples/pr-review-bedrock.yml index 374cfc9..29374e2 100644 --- a/examples/pr-review-bedrock.yml +++ b/examples/pr-review-bedrock.yml @@ -1,17 +1,33 @@ # .github/workflows/pr-review.yml # -# Bedrock PR review (modsy standard). Authenticates to AWS via GitHub OIDC, -# no API token to manage. Advisory, non-blocking. Uses a non-Claude model -# (Amazon Nova) by default for reviewer independence. +# Bedrock PR review (modsy standard). Authenticates to AWS via GitHub OIDC, no +# API token to manage. Advisory, non-blocking. The finding pass defaults to a +# non-Claude model (GPT-5.6 Terra) so the reviewer is independent of the Claude +# Code that authors the change; the adjudicator that rechecks each finding +# defaults to Sonnet 4.6. # # Copy to .github/workflows/pr-review.yml in your repo and set aws_role_arn to # a role that allows bedrock:Converse (trusted for your repo via OIDC). +# +# The issue_comment and pull_request_review_comment triggers are what make +# on-demand review work: +# +# @bedrock-review re-review the current HEAD +# @bedrock-review dismiss delete every finding this reviewer posted +# +# Drop those two triggers if you only want automatic review on push. Keep the +# phrase in the `if` below identical to the reviewer's trigger_phrase input, +# or comments will spin up a runner only to be ignored. name: PR Review on: pull_request: types: [opened, synchronize, reopened, ready_for_review] + issue_comment: + types: [created] + pull_request_review_comment: + types: [created] permissions: contents: read @@ -21,9 +37,23 @@ permissions: jobs: review: - if: github.event.pull_request.draft == false + # Draft PRs are skipped by the reusable workflow itself, so do not filter + # them here: `github.event.pull_request` is absent on issue_comment events, + # and a draft check at this level would swallow every on-demand request. + if: >- + github.event_name == 'pull_request' || + (github.event_name == 'issue_comment' && + github.event.issue.pull_request && + contains(github.event.comment.body, '@bedrock-review')) || + (github.event_name == 'pull_request_review_comment' && + contains(github.event.comment.body, '@bedrock-review')) uses: modsy/ci-workflows/.github/workflows/bedrock-pr-review.yml@v1.0.0 with: - pr_number: ${{ github.event.pull_request.number }} aws_role_arn: arn:aws:iam::052457202381:role/github-bedrock-pr-review - # model_id: us.amazon.nova-pro-v1:0 # override if needed + # pr_number is resolved from the triggering event; pass it only for + # workflow_dispatch. + # + # model_id: us.amazon.nova-pro-v1:0 # override the finding model + # min_confidence: high # post only high-confidence findings + # adjudicate: false # skip the verification pass + # review_guide_path: .github/review-guide.md diff --git a/scripts/validate-workflows.py b/scripts/validate-workflows.py new file mode 100755 index 0000000..d587a00 --- /dev/null +++ b/scripts/validate-workflows.py @@ -0,0 +1,111 @@ +#!/usr/bin/env python3 +"""Validate every workflow in this repo and the scripts embedded in it. + +A reusable workflow only fails when a consumer dispatches it, so a syntax error +in an embedded `run:` block or `github-script` body is invisible here until it +breaks somebody else's PR. This checks all three languages up front: + + - the workflow file parses as YAML (a raw control character does not) + - every `run:` block parses as bash + - every `<<'PY'` heredoc inside a `run:` block parses as Python + - every `actions/github-script` body parses as JavaScript + +Requires: bash, python3, node. Run from the repo root. +""" + +from __future__ import annotations + +import re +import subprocess +import sys +import tempfile +from pathlib import Path + +import yaml + +WORKFLOWS = Path(".github/workflows") +# A NUL or other C0 control character parses fine as JavaScript but makes the +# surrounding YAML unloadable, which is how this check earned its place. +CONTROL_CHARS = re.compile(r"[\x00-\x08\x0b\x0c\x0e-\x1f]") +PY_HEREDOC = re.compile(r"<<'PY'\n(.*?)\n\s*PY(?:\n|$)", re.S) + +failures: list[str] = [] + + +def fail(where: str, detail: str) -> None: + failures.append(f"{where}: {detail}") + print(f"FAIL {where}: {detail}") + + +def check_syntax(where: str, source: str, argv: list[str], suffix: str) -> None: + with tempfile.NamedTemporaryFile("w", suffix=suffix, delete=False) as handle: + handle.write(source) + temp = handle.name + result = subprocess.run([*argv, temp], capture_output=True, text=True) + if result.returncode != 0: + fail(where, (result.stderr or result.stdout).strip().splitlines()[0] if (result.stderr or result.stdout).strip() else "syntax error") + Path(temp).unlink(missing_ok=True) + + +def check_python(where: str, source: str) -> None: + try: + compile(source, where, "exec") + except SyntaxError as exc: + fail(where, f"line {exc.lineno}: {exc.msg}") + + +def main() -> int: + files = sorted(WORKFLOWS.glob("*.yml")) + sorted(WORKFLOWS.glob("*.yaml")) + if not files: + print(f"No workflows found under {WORKFLOWS}", file=sys.stderr) + return 1 + + for path in files: + raw = path.read_text(encoding="utf-8") + found = CONTROL_CHARS.search(raw) + if found: + fail(str(path), f"control character {found.group(0)!r} at offset {found.start()}") + continue + + try: + document = yaml.safe_load(raw) + except yaml.YAMLError as exc: + fail(str(path), f"invalid YAML: {exc}") + continue + + jobs = (document or {}).get("jobs") or {} + for job_name, job in jobs.items(): + for index, step in enumerate(job.get("steps") or []): + label = step.get("name") or f"step {index}" + where = f"{path}:{job_name}:{label}" + + run = step.get("run") + if isinstance(run, str): + check_syntax(f"{where} (bash)", run, ["bash", "-n"], ".sh") + for n, block in enumerate(PY_HEREDOC.findall(run)): + check_python(f"{where} (python block {n})", block) + + uses = step.get("uses") or "" + if uses.startswith("actions/github-script"): + body = (step.get("with") or {}).get("script") + if isinstance(body, str): + # github-script runs the body inside an async wrapper, so + # top-level await is legal there and must be here too. + check_syntax( + f"{where} (javascript)", + "(async () => {\n" + body + "\n})();\n", + ["node", "--check"], + ".js", + ) + + print(f"OK {path}") + + if failures: + print(f"\n{len(failures)} failure(s).") + return 1 + print(f"\nAll {len(files)} workflow(s) valid.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/reconcile.test.mjs b/tests/reconcile.test.mjs new file mode 100644 index 0000000..17a3a61 --- /dev/null +++ b/tests/reconcile.test.mjs @@ -0,0 +1,366 @@ +// Unit tests for the reconcile logic embedded in bedrock-pr-review.yml. +// +// The logic under test is extracted verbatim from the workflow file at test +// time, so there is no second copy to drift: editing the workflow's PURE LOGIC +// region is what these tests run. Run with `node --test tests/`. + +import { readFileSync } from "node:fs"; +import { strict as assert } from "node:assert"; +import test from "node:test"; + +const WORKFLOW = new URL("../.github/workflows/bedrock-pr-review.yml", import.meta.url); +const source = readFileSync(WORKFLOW, "utf8"); + +const REGION_RE = /\/\/ ===== BEGIN PURE LOGIC =====\n([\s\S]*?)\/\/ ===== END PURE LOGIC =====/; + +function extractRegion() { + const match = source.match(REGION_RE); + if (!match) { + throw new Error( + "PURE LOGIC region not found in bedrock-pr-review.yml. If the markers were " + + "renamed, update REGION_RE here so the logic stays under test." + ); + } + // The region sits inside a YAML block scalar, indented 12 spaces. + return match[1] + .split("\n") + .map((line) => (line.startsWith(" ".repeat(12)) ? line.slice(12) : line)) + .join("\n"); +} + +const EXPORTS = [ + "SEVERITY_RANK", "CONFIDENCE_RANK", "VARIANT_LINE_WINDOW", "LIVE_VARIANT_LINE_WINDOW", + "VARIANT_SIMILARITY_THRESHOLD", "STICKY_LINE_WINDOW", + "normalizeAnchor", "firstSentence", "stripMarkers", "extractFingerprint", "fingerprint", + "claimTokens", "jaccard", "bestVariantMatch", "meetsFloor", "lineChanged", "anchorOccurrence", + "assertedFingerprints", "ANY_MARKER_RE", "FINGERPRINT_MARKER_RE", +]; + +const module_ = await import( + "data:text/javascript," + + encodeURIComponent( + 'import crypto from "node:crypto";\n' + + extractRegion() + + `\nexport { ${EXPORTS.join(", ")} };\n` + ) +); + +const { + VARIANT_LINE_WINDOW, LIVE_VARIANT_LINE_WINDOW, VARIANT_SIMILARITY_THRESHOLD, STICKY_LINE_WINDOW, + normalizeAnchor, extractFingerprint, fingerprint, + claimTokens, jaccard, bestVariantMatch, meetsFloor, lineChanged, anchorOccurrence, + assertedFingerprints, ANY_MARKER_RE, +} = module_; + +// --- the workflow file itself ------------------------------------------------- + +test("workflow file carries no control characters that break YAML", () => { + // A raw NUL in the embedded JS parses as JavaScript but makes the whole + // workflow file invalid YAML, which fails at dispatch rather than in review. + const offending = source.match(/[\u0000-\u0008\u000B\u000C\u000E-\u001F]/); + assert.equal(offending, null, `found control character ${JSON.stringify(offending && offending[0])}`); +}); + +test("tuning constants match the calibrated FinalWalk values", () => { + assert.equal(VARIANT_LINE_WINDOW, 10); + assert.equal(LIVE_VARIANT_LINE_WINDOW, 40); + assert.equal(VARIANT_SIMILARITY_THRESHOLD, 0.35); + assert.equal(STICKY_LINE_WINDOW, 3); +}); + +// --- identity ----------------------------------------------------------------- + +const at = (line, extra = {}) => + Object.assign({ path: "src/app.py", line, lineAnchor: " total = sum(values)", body: "b" }, extra); + +test("identity ignores the line number, so an unrelated edit above does not move it", () => { + assert.equal(fingerprint(at(42)), fingerprint(at(97))); +}); + +test("identity changes when the anchored code changes", () => { + assert.notEqual( + fingerprint(at(42)), + fingerprint(at(42, { lineAnchor: " total = mean(values)" })) + ); +}); + +test("identity survives the model rewording its finding", () => { + const original = fingerprint(at(42, { body: "issue: sums instead of averaging" })); + const reworded = fingerprint(at(42, { body: "issue: this adds the values rather than taking their mean" })); + assert.equal(original, reworded); +}); + +test("identity is scoped to the path", () => { + assert.notEqual(fingerprint(at(42)), fingerprint(at(42, { path: "src/other.py" }))); +}); + +test("identity normalizes whitespace in the anchor", () => { + assert.equal( + fingerprint(at(42, { lineAnchor: " total =\tsum(values) " })), + fingerprint(at(42, { lineAnchor: "total = sum(values)" })) + ); +}); + +test("identity falls back to anchor_text, then to the body's first sentence", () => { + const viaAnchorText = fingerprint({ + path: "a.py", line: 1, lineAnchor: "", anchor_text: "x = 1", body: "issue: bad. more prose.", + }); + assert.equal(viaAnchorText, fingerprint({ path: "a.py", line: 9, lineAnchor: "", anchor_text: "x = 1", body: "different" })); + + const viaSentence = fingerprint({ path: "a.py", line: 1, lineAnchor: "", anchor_text: "", body: "issue: bad. more prose." }); + assert.equal(viaSentence, fingerprint({ path: "a.py", line: 4, lineAnchor: "", anchor_text: "", body: "issue: bad. entirely other tail." })); + assert.notEqual(viaSentence, viaAnchorText); +}); + +test("occurrence index separates two findings on identical lines", () => { + assert.notEqual(fingerprint(at(42, { occurrence: 0 })), fingerprint(at(88, { occurrence: 1 }))); +}); + +test("occurrence zero and absent occurrence produce the same identity", () => { + assert.equal(fingerprint(at(42)), fingerprint(at(42, { occurrence: 0 }))); +}); + +test("extractFingerprint round-trips the hidden marker the workflow writes", () => { + const digest = fingerprint(at(42)); + assert.equal(extractFingerprint(`body text\n\n`), digest); + assert.equal(extractFingerprint("no marker here"), ""); + assert.equal(extractFingerprint(undefined), ""); +}); + +test("anchorOccurrence counts identical lines and ignores unrelated ones", () => { + const lines = [' "a": 1,', ' "b": 2,', ' "a": 1,', "", ' "a": 1,']; + assert.equal(anchorOccurrence(lines, 1), 0); + assert.equal(anchorOccurrence(lines, 3), 1); + assert.equal(anchorOccurrence(lines, 5), 2); + assert.equal(anchorOccurrence(lines, 2), 0, "a unique line is always occurrence 0"); + assert.equal(anchorOccurrence(lines, 4), 0, "a blank line has no anchor"); +}); + +// --- claim similarity --------------------------------------------------------- + +test("claimTokens keeps identifiers and code spans and drops generic review vocabulary", () => { + const tokens = claimTokens("issue: the `spec_id` value is not checked, so this could return None"); + assert.ok(tokens.has("spec_id"), "backticked identifier is the strongest signal"); + assert.ok(tokens.has("none")); + for (const generic of ["the", "value", "not", "checked", "could", "return", "this", "issue"]) { + assert.ok(!tokens.has(generic), `"${generic}" should be a stopword`); + } +}); + +test("claimTokens ignores hidden markers so a posted body scores like a fresh one", () => { + const fresh = claimTokens("issue: `spec_id` unchecked"); + const posted = claimTokens("issue: `spec_id` unchecked\n\n"); + assert.deepEqual([...posted].sort(), [...fresh].sort()); +}); + +test("jaccard is 1 for identical sets, 0 for disjoint, 0 for empty", () => { + assert.equal(jaccard(new Set(["a", "b"]), new Set(["a", "b"])), 1); + assert.equal(jaccard(new Set(["a"]), new Set(["b"])), 0); + assert.equal(jaccard(new Set(), new Set(["a"])), 0); + assert.equal(jaccard(new Set(["a", "b"]), new Set(["a"])), 0.5); +}); + +const dismissal = (line, body) => ({ path: "src/app.py", line, tokens: claimTokens(body), fp: "deadbeef" }); + +const REWORDED_A = "issue: `deposit_rate` is hardcoded to 0.10 while each deal carries its own deposit terms"; +const REWORDED_B = "issue: the hardcoded 0.10 `deposit_rate` ignores per-deal deposit terms on non-default deals"; +const UNRELATED = "suggestion: `render_pdf` opens the file without a timeout, so a slow disk stalls the request"; + +test("a reworded restatement inside the window matches its dismissal", () => { + const match = bestVariantMatch( + { path: "src/app.py", line: 44, body: REWORDED_B }, + [dismissal(40, REWORDED_A)], + VARIANT_LINE_WINDOW + ); + assert.ok(match, "reworded variant should match"); + assert.ok(match.score >= VARIANT_SIMILARITY_THRESHOLD); +}); + +test("the same restatement outside the window does not match", () => { + assert.equal( + bestVariantMatch( + { path: "src/app.py", line: 200, body: REWORDED_B }, + [dismissal(40, REWORDED_A)], + VARIANT_LINE_WINDOW + ), + null + ); +}); + +test("an unrelated finding on an adjacent line does not match", () => { + assert.equal( + bestVariantMatch( + { path: "src/app.py", line: 41, body: UNRELATED }, + [dismissal(40, REWORDED_A)], + VARIANT_LINE_WINDOW + ), + null + ); +}); + +test("a finding in another file never matches", () => { + assert.equal( + bestVariantMatch( + { path: "src/other.py", line: 40, body: REWORDED_B }, + [dismissal(40, REWORDED_A)], + VARIANT_LINE_WINDOW + ), + null + ); +}); + +test("a finding with no real line anchor gets exact matching only", () => { + assert.equal( + bestVariantMatch({ path: "src/app.py", line: 0, body: REWORDED_B }, [dismissal(40, REWORDED_A)], VARIANT_LINE_WINDOW), + null, + "line 0 cannot establish locality, so it must not match on proximity" + ); +}); + +test("the highest-scoring dismissal wins when several qualify", () => { + const near = dismissal(41, REWORDED_A); + const exactish = dismissal(43, REWORDED_B); + const match = bestVariantMatch( + { path: "src/app.py", line: 42, body: REWORDED_B }, + [near, exactish], + VARIANT_LINE_WINDOW + ); + assert.ok(match); + assert.equal(match.record.line, 43, "must pick the more similar record, not the closer one"); +}); + +test("the live-thread window is wide enough for the drift it was calibrated on", () => { + const candidate = { path: "src/app.py", line: 73, body: REWORDED_B }; + const records = [dismissal(40, REWORDED_A)]; + assert.equal(bestVariantMatch(candidate, records, VARIANT_LINE_WINDOW), null); + assert.ok(bestVariantMatch(candidate, records, LIVE_VARIANT_LINE_WINDOW)); +}); + +// --- floors ------------------------------------------------------------------- + +test("confidence below the floor is dropped and at or above it is kept", () => { + const { CONFIDENCE_RANK } = module_; + assert.equal(meetsFloor(CONFIDENCE_RANK, "low", "medium"), false); + assert.equal(meetsFloor(CONFIDENCE_RANK, "medium", "medium"), true); + assert.equal(meetsFloor(CONFIDENCE_RANK, "high", "medium"), true); + assert.equal(meetsFloor(CONFIDENCE_RANK, "HIGH", "medium"), true, "grade is case-insensitive"); + assert.equal(meetsFloor(CONFIDENCE_RANK, "low", "low"), true); +}); + +test("a missing or unknown grade is treated as the floor, never silently dropped", () => { + const { CONFIDENCE_RANK, SEVERITY_RANK } = module_; + assert.equal(meetsFloor(CONFIDENCE_RANK, undefined, "medium"), true); + assert.equal(meetsFloor(CONFIDENCE_RANK, "", "high"), true); + assert.equal(meetsFloor(CONFIDENCE_RANK, "vibes", "high"), true); + assert.equal(meetsFloor(SEVERITY_RANK, undefined, "blocking"), true); +}); + +test("severity floor keeps nitpicks out of threads at the default setting", () => { + const { SEVERITY_RANK } = module_; + assert.equal(meetsFloor(SEVERITY_RANK, "nitpick", "suggestion"), false); + assert.equal(meetsFloor(SEVERITY_RANK, "suggestion", "suggestion"), true); + assert.equal(meetsFloor(SEVERITY_RANK, "blocking", "suggestion"), true); +}); + +// --- sticky threads ----------------------------------------------------------- + +const RANGES = { "src/app.py": [[100, 104], [200, 200]] }; + +test("a line inside a changed hunk counts as changed", () => { + assert.equal(lineChanged(RANGES, "src/app.py", 102, STICKY_LINE_WINDOW), true); +}); + +test("a line just outside a changed hunk still counts, within the window", () => { + assert.equal(lineChanged(RANGES, "src/app.py", 97, STICKY_LINE_WINDOW), true); + assert.equal(lineChanged(RANGES, "src/app.py", 107, STICKY_LINE_WINDOW), true); +}); + +test("a line well clear of every changed hunk counts as unchanged, so its thread is kept", () => { + assert.equal(lineChanged(RANGES, "src/app.py", 150, STICKY_LINE_WINDOW), false); + assert.equal(lineChanged(RANGES, "src/app.py", 1, STICKY_LINE_WINDOW), false); +}); + +test("a file with no recorded changes counts as unchanged", () => { + assert.equal(lineChanged(RANGES, "src/untouched.py", 100, STICKY_LINE_WINDOW), false); + assert.equal(lineChanged({}, "src/app.py", 100, STICKY_LINE_WINDOW), false); +}); + +test("normalizeAnchor handles null and undefined without throwing", () => { + assert.equal(normalizeAnchor(null), ""); + assert.equal(normalizeAnchor(undefined), ""); +}); + +// --- marker ownership --------------------------------------------------------- + +test("the marker regex claims every marker this reviewer writes", () => { + // Ownership drives both the dismiss sweep and the reconcile pass. A marker the + // regex does not claim is a comment the reviewer can neither update nor delete. + for (const body of [ + "summary\n", + "finding\n", + "summary\n", + "\nDismissed 3 review comment(s).", + ]) { + assert.ok(ANY_MARKER_RE.test(body), `must claim: ${JSON.stringify(body)}`); + } +}); + +test("the marker regex claims the summary marker on its own", () => { + // The summary carries a reviewed-sha stamp too, so a regex that missed the + // bedrock-reviewer marker would still appear to work until head_sha resolved + // empty and the stamp was omitted. + assert.ok(ANY_MARKER_RE.test("## Pull request overview\n")); +}); + +test("the marker regex does not claim another reviewer's comments", () => { + // The Codex reviewer also posts as github-actions[bot], so the marker is the + // only thing separating the two. Claiming an unmarked bot comment would let + // this workflow delete the other reviewer's threads. + for (const body of [ + "issue: something is wrong", + "", + "", + "", + ]) { + assert.ok(!ANY_MARKER_RE.test(body), `must not claim: ${JSON.stringify(body)}`); + } +}); + +// --- thread preservation ------------------------------------------------------ + +const finding = (fp) => ({ fp, path: "src/app.py", line: 40 }); + +test("a finding restated this run keeps its thread, whichever bucket it lands in", () => { + // The bucket a finding is presented in (inline thread, minor note, non-diff + // note, or dropped by a confidence floor) must not decide whether its thread + // survives: only whether the claim was restated at all. + const existing = new Set(["aaa", "bbb", "ccc"]); + const asserted = assertedFingerprints([finding("aaa"), finding("ccc")], existing, new Set()); + assert.deepEqual([...asserted].sort(), ["aaa", "ccc"]); + assert.ok(!asserted.has("bbb"), "a claim nobody restated stays a deletion candidate"); +}); + +test("a thread that absorbed a reworded variant is kept, not deleted", () => { + // The variant reconcile drops the new candidate to avoid a duplicate. If the + // thread it reconciled into were not marked, the orphan sweep would delete it + // on the same run and the claim would vanish entirely. + const asserted = assertedFingerprints([], new Set(["aaa"]), new Set(["aaa"])); + assert.ok(asserted.has("aaa")); +}); + +test("a finding with no matching thread does not invent one", () => { + const asserted = assertedFingerprints([finding("zzz")], new Set(["aaa"]), new Set()); + assert.equal(asserted.size, 0); +}); + +test("a finding with no fingerprint is ignored rather than throwing", () => { + const asserted = assertedFingerprints( + [{ path: "a.py", line: 1 }, { fp: "", path: "a.py", line: 2 }], new Set(["aaa"]), new Set()); + assert.equal(asserted.size, 0); +}); + +test("no findings at all leaves every thread a deletion candidate", () => { + // A run that legitimately finds nothing must still be able to clean up, but + // only via the sticky rule, which is tested separately. + assert.equal(assertedFingerprints([], new Set(["aaa", "bbb"]), new Set()).size, 0); +}); From d1dd281e891251bb0174f80f7f371186635a3158 Mon Sep 17 00:00:00 2001 From: aprilb <458678+aprilb@users.noreply.github.com> Date: Wed, 2 Sep 2026 11:30:54 -0700 Subject: [PATCH 2/2] fix(bedrock-pr-review): address six review findings Self-review of the previous commit surfaced six defects. All are fixed here, each with a test that fails when the fix is reverted. 1. Nothing before the model call may red the advisory check. The OIDC role assumption, Python setup, SDK install, caller write and the post step had no continue-on-error, so a repo missing from the reviewer role's trust policy red-Xed every PR in it, contradicting the fail-soft guarantee this workflow states in its own header. All are fail-soft now, a Check setup step names what failed, and the run summary reports it instead of letting it surface as a confusing model error. 2. On-demand and dismiss now require write access. Both are privileged: dismiss deletes findings, and on-demand spends against the caller's Bedrock role while deliberately bypassing the debounce and already-reviewed gates. Anyone can comment on a PR, so a drive-by commenter could previously delete every finding or drive unbounded spend. Gated on the comment's author_association being OWNER, MEMBER or COLLABORATOR, with a ::notice:: naming a denied association so it is visible rather than silent. 3. The adjudicator can re-grade what it rescues. The confidence floor runs after adjudication, and a verdict could not change confidence, so a hedged finding the judge confirmed and rewrote into an assertion was then discarded by the default medium floor: precisely the candidate the adjudication pass exists to rescue. A verdict may now carry a confidence, and a rewritten candidate is floored at medium on the grounds that the rewrite IS the assertion. 4. max_files uses the exact changed-file count. It was derived from `gh pr view --json files`, which resolves through GraphQL `files(first: 100)` and saturates at 100, so any cap above that never fired: a 600-file PR reported 100 and sailed through a cap of 150. Read `.changed_files` from the REST pulls resource instead, and fail the guard open on a malformed value. 5. Comment events dedupe by PR. They were isolated by run_id, so two `@bedrock-review` comments ran at once, neither saw the other's posts, both opened a thread per finding, and the loser was unreachable forever after because the reconcile pass indexes one comment per fingerprint. Now keyed on the PR plus an event kind, so command runs dedupe with each other while still never cancelling a push review. 6. The model's line number is coerced once at ingest. A string "42" failed the commentable-line Set lookup and demoted the finding to a summary bullet even when its line was in the diff, and would have failed the strict equality that decides whether a finding moved, delete-and-reposting an unmoved comment on every run. A finding with an unusable line is now dropped with a warning rather than silently mishandled. Tests: 89, up from 38. Two new suites, both extracting the code under test from the workflow YAML so there is still only one copy: tests/trigger.test.mjs covers the author-association gate and that a hostile comment body cannot reach the shell, and tests/adjudication.test.mjs covers the verdict applier's keep, drop, rewrite, re-grade and fail-open paths. reconcile.test.mjs gains workflow-shape guards for the concurrency group, the fail-soft chain and the exact file count. Mutation-checked: reverting any one of the six fixes fails at least one test. --- .github/workflows/bedrock-pr-review.yml | 186 +++++++++++++++++--- README.md | 26 ++- tests/adjudication.test.mjs | 217 ++++++++++++++++++++++++ tests/reconcile.test.mjs | 90 +++++++++- tests/trigger.test.mjs | 190 +++++++++++++++++++++ 5 files changed, 675 insertions(+), 34 deletions(-) create mode 100644 tests/adjudication.test.mjs create mode 100644 tests/trigger.test.mjs diff --git a/.github/workflows/bedrock-pr-review.yml b/.github/workflows/bedrock-pr-review.yml index b1e43d1..0fff4e8 100644 --- a/.github/workflows/bedrock-pr-review.yml +++ b/.github/workflows/bedrock-pr-review.yml @@ -177,16 +177,23 @@ jobs: github.event.pull_request.user.login != 'dependabot[bot]' concurrency: - # Only PR code-change events dedupe each other, so a new push cancels a - # stale in-flight review. Comment events (including this reviewer's own - # inline comments, which fire pull_request_review_comment) get an isolated - # group via run_id and never cancel a running review. + # Two dimensions: the PR, and whether this run came from a code change or + # from a comment. A new push cancels a stale in-flight review, and a second + # on-demand request cancels the first instead of racing it. Comment runs + # never share a group with push runs, so this reviewer's own inline + # comments (which fire pull_request_review_comment) cannot cancel a review + # in flight. + # + # Comment events MUST dedupe by PR. Keying them on run_id instead lets two + # `@bedrock-review` comments run at once; neither sees the other's posts, + # both open a thread per finding, and the loser is unreachable forever + # after, because the reconcile pass indexes one comment per fingerprint. group: >- bedrock-pr-review-${{ github.repository }}-${{ - github.event_name == 'pull_request' - && github.event.pull_request.number - || github.run_id - }} + github.event.pull_request.number + || github.event.issue.number + || inputs.pr_number + }}-${{ github.event_name == 'pull_request' && 'push' || 'command' }} cancel-in-progress: true permissions: @@ -228,11 +235,12 @@ jobs: env: EVENT_NAME: ${{ github.event_name }} COMMENT_BODY: ${{ github.event.comment.body }} + COMMENT_ASSOCIATION: ${{ github.event.comment.author_association }} IS_PR_COMMENT: ${{ github.event.issue.pull_request && 'true' || 'false' }} TRIGGER_PHRASE: ${{ inputs.trigger_phrase }} run: | set -euo pipefail - command="review" + command="review"; denied_association="" case "$EVENT_NAME" in issue_comment|pull_request_review_comment) # An issue_comment fires for plain issues too; only PR comments count. @@ -240,19 +248,38 @@ jobs: command="ignore" elif [ -z "${TRIGGER_PHRASE:-}" ]; then command="ignore" - elif printf '%s' "${COMMENT_BODY:-}" | grep -qF -- "${TRIGGER_PHRASE} dismiss"; then - command="dismiss" - elif printf '%s' "${COMMENT_BODY:-}" | grep -qF -- "${TRIGGER_PHRASE}"; then - command="on-demand" - else + elif ! printf '%s' "${COMMENT_BODY:-}" | grep -qF -- "${TRIGGER_PHRASE}"; then command="ignore" + else + # Both commands are privileged: dismiss deletes findings, and + # on-demand spends against the caller's Bedrock role while + # deliberately bypassing the debounce and already-reviewed gates. + # Anyone can comment on a PR, so require write access. GitHub + # reports that as the comment's author_association. + case "${COMMENT_ASSOCIATION:-NONE}" in + OWNER|MEMBER|COLLABORATOR) + if printf '%s' "${COMMENT_BODY:-}" | grep -qF -- "${TRIGGER_PHRASE} dismiss"; then + command="dismiss" + else + command="on-demand" + fi + ;; + *) + command="ignore" + denied_association="${COMMENT_ASSOCIATION:-NONE}" + ;; + esac fi ;; esac eligible="false" case "$command" in review|on-demand) eligible="true" ;; esac - echo "command=$command" >> "$GITHUB_OUTPUT" - echo "eligible=$eligible" >> "$GITHUB_OUTPUT" + echo "command=$command" >> "$GITHUB_OUTPUT" + echo "eligible=$eligible" >> "$GITHUB_OUTPUT" + echo "denied=$denied_association" >> "$GITHUB_OUTPUT" + if [ -n "$denied_association" ]; then + echo "::notice::Ignoring '${TRIGGER_PHRASE}' from a commenter without write access (author_association: ${denied_association})." + fi echo "Trigger classified as: $command" # --------------------------------------------------------------- @@ -331,7 +358,7 @@ jobs: set -euo pipefail pr_json="$RUNNER_TEMP/pr.json" gh pr view "$PR_NUMBER" \ - --json number,title,body,url,baseRefName,baseRefOid,headRefName,headRefOid,isDraft,author,labels,files \ + --json number,title,body,url,baseRefName,baseRefOid,headRefName,headRefOid,isDraft,author,labels \ > "$pr_json" base_ref="$(jq -r '.baseRefName' "$pr_json")" @@ -339,7 +366,13 @@ jobs: head_sha="$(git rev-parse 'HEAD^2' 2>/dev/null || jq -r '.headRefOid' "$pr_json")" is_draft="$(jq -r '.isDraft' "$pr_json")" paused="$(jq -r '[.labels[].name] | index("codex:pause") | if . == null then "false" else "true" end' "$pr_json")" - file_count="$(jq -r '.files | length' "$pr_json")" + # Exact count from the REST resource. `gh pr view --json files` resolves + # through GraphQL `files(first: 100)`, so it saturates at 100 and would + # silently defeat any max_files above that. An empty result (API hiccup) + # is treated as 0, which fails the guard open rather than skipping a PR + # that might be small. + file_count="$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}" --jq '.changed_files' 2>/dev/null || true)" + [[ "$file_count" =~ ^[0-9]+$ ]] || file_count=0 # File-count guard. A PR far over the cap blows the diff budget and # yields shallow findings, so skip rather than review it badly. @@ -520,21 +553,33 @@ jobs: # --------------------------------------------------------------- # 5. AWS auth via GitHub OIDC (no static keys). # --------------------------------------------------------------- + # Every setup step here is fail-soft. This reviewer is advisory, so a + # misconfiguration must degrade to "no review", never to a red check on the + # consumer's PR. The most likely failure by far is the OIDC role: until a + # repo is added to the role's trust policy the assume-role call returns + # AccessDenied, and without continue-on-error that single missing trust + # entry red-Xes every PR in the repo. - name: Configure AWS credentials (OIDC) + id: aws if: steps.gate.outputs.proceed == 'true' + continue-on-error: true uses: aws-actions/configure-aws-credentials@v4 with: role-to-assume: ${{ inputs.aws_role_arn }} aws-region: ${{ inputs.aws_region }} - name: Set up Python + id: python if: steps.gate.outputs.proceed == 'true' + continue-on-error: true uses: actions/setup-python@v5 with: python-version: "3.12" - name: Install SDKs + id: sdks if: steps.gate.outputs.proceed == 'true' + continue-on-error: true run: pip install --quiet boto3 openai aws-bedrock-token-generator # --------------------------------------------------------------- @@ -543,7 +588,9 @@ jobs: # implemented once. # --------------------------------------------------------------- - name: Write Bedrock caller + id: caller if: steps.gate.outputs.proceed == 'true' + continue-on-error: true shell: bash run: | set -euo pipefail @@ -593,12 +640,39 @@ jobs: PY python3 -c "import ast,sys; ast.parse(open(sys.argv[1]).read())" "$RUNNER_TEMP/bedrock_call.py" + # --------------------------------------------------------------- + # 5c. Did setup survive? Reported so a failed run says "auth" or "deps" + # rather than surfacing as a confusing model error. + # --------------------------------------------------------------- + - name: Check setup + id: setup + if: always() && steps.gate.outputs.proceed == 'true' + shell: bash + env: + AWS_OUTCOME: ${{ steps.aws.outcome }} + PYTHON_OUTCOME: ${{ steps.python.outcome }} + SDKS_OUTCOME: ${{ steps.sdks.outcome }} + CALLER_OUTCOME: ${{ steps.caller.outcome }} + run: | + set -euo pipefail + ok="true"; failed="" + [ "${AWS_OUTCOME}" = "success" ] || { ok="false"; failed="${failed}AWS OIDC role assumption, "; } + [ "${PYTHON_OUTCOME}" = "success" ] || { ok="false"; failed="${failed}Python setup, "; } + [ "${SDKS_OUTCOME}" = "success" ] || { ok="false"; failed="${failed}SDK install, "; } + [ "${CALLER_OUTCOME}" = "success" ] || { ok="false"; failed="${failed}Bedrock caller, "; } + failed="${failed%, }" + echo "ok=${ok}" >> "$GITHUB_OUTPUT" + echo "failed=${failed}" >> "$GITHUB_OUTPUT" + if [ "${ok}" != "true" ]; then + echo "::warning::Setup failed (${failed}); skipping the review. Advisory check stays green." + fi + # --------------------------------------------------------------- # 6. Finding pass. Fail-soft: a model/auth error never fails the check. # --------------------------------------------------------------- - name: Run Bedrock review id: bedrock - if: steps.gate.outputs.proceed == 'true' + if: steps.gate.outputs.proceed == 'true' && steps.setup.outputs.ok == 'true' continue-on-error: true shell: bash env: @@ -799,11 +873,17 @@ jobs: Respond with ONLY a JSON object between the markers, no prose outside them: BEGIN_ADJUDICATION_JSON - {"verdicts": [{"id": "c0", "keep": true, "reason": "...", "rewritten_body": ""}]} + {"verdicts": [{"id": "c0", "keep": true, "reason": "...", "rewritten_body": "", "confidence": "high"}]} END_ADJUDICATION_JSON - `rewritten_body` is optional; leave it empty to keep the candidate's own wording. A verdict - never changes a candidate's path or line.""" + `rewritten_body` is optional; leave it empty to keep the candidate's own wording. + `confidence` is optional and is YOUR grade for the finding after verification: "high", + "medium" or "low". Set it whenever your verdict disagrees with the candidate's own + confidence, and in particular when you rewrote a hedged candidate into an assertion, + because you have just established the failure the candidate was only asking about. + Findings below the run's confidence floor are discarded before posting, so leaving a + rescued candidate at its original low grade throws away the work you just did. + A verdict never changes a candidate's path or line.""" instructions = "\n".join(line.strip() for line in instructions.splitlines()) parts = [instructions, ""] @@ -875,6 +955,19 @@ jobs: rewritten = str(verdict.get("rewritten_body") or "").strip() if rewritten: c["body"] = rewritten + graded = str(verdict.get("confidence") or "").strip().lower() + if graded in {"low", "medium", "high"}: + c["confidence"] = graded + elif rewritten: + # The judge turned a hedge into an assertion but did not re-grade. + # Keeping the worker's low grade would let the confidence floor + # discard the finding the adjudication pass exists to rescue, so + # floor it at medium: the rewrite IS the assertion. + rank = {"low": 1, "medium": 2, "high": 3} + if rank.get(str(c.get("confidence", "medium")).lower(), 2) < 2: + c["confidence"] = "medium" + print(f"::notice::Re-graded {c.get('path')}:{c.get('line')} to medium: " + "the adjudicator rewrote it into an assertion") kept.append(c) review["comments"] = kept @@ -898,6 +991,10 @@ jobs: - name: Post Bedrock review id: post_review if: always() && steps.gate.outputs.proceed == 'true' && steps.bedrock.outcome == 'success' + # Fail-soft for the same reason as the setup steps: a secondary rate limit + # on the comment APIs must not red-X the consumer's PR. The run summary + # below reports the failure loudly so it is not swallowed. + continue-on-error: true uses: actions/github-script@v7 env: PR_NUMBER: ${{ steps.pr.outputs.number }} @@ -1075,6 +1172,16 @@ jobs: return spans.some(function (span) { return span[0] <= hi && span[1] >= lo; }); } + // The model's `line` arrives from JSON and is only asked, not guaranteed, + // to be a number. A string "42" fails the commentable-line Set lookup + // (a Set of numbers) and fails the strict equality that decides whether a + // finding moved, so it is coerced once here rather than at each use. + // Returns 0 for anything that is not a usable 1-based line number. + function parseLine(value) { + const line = typeof value === "number" ? value : Number(String(value == null ? "" : value).trim()); + return Number.isInteger(line) && line > 0 ? line : 0; + } + // Which existing threads does this run still assert? A thread survives if // any finding that got past dismissal carries its fingerprint, whichever // presentation bucket that finding landed in (inline thread, minor note, @@ -1178,11 +1285,19 @@ jobs: const candidates = []; for (const c of rawComments) { - if (!c.path || !c.line || !c.body) continue; + const line = parseLine(c.line); + if (!c.path || !line || !c.body) { + if (c.path && c.body && !line) { + core.warning("Dropping a finding on " + c.path + " with an unusable line " + + JSON.stringify(c.line)); + } + continue; + } const lines = fileLines(c.path); - const lineAnchor = lines && lines[c.line - 1] !== undefined ? lines[c.line - 1] : ""; - const occurrence = lines && lineAnchor ? anchorOccurrence(lines, c.line) : 0; + const lineAnchor = lines && lines[line - 1] !== undefined ? lines[line - 1] : ""; + const occurrence = lines && lineAnchor ? anchorOccurrence(lines, line) : 0; candidates.push(Object.assign({}, c, { + line: line, lineAnchor: lineAnchor, occurrence: occurrence, fp: fingerprint({ @@ -1385,7 +1500,7 @@ jobs: const existing = existingByFp.get(c.fp); if (existing) { matchedFps.add(c.fp); - const existingLine = existing.line == null ? existing.original_line : existing.line; + const existingLine = parseLine(existing.line == null ? existing.original_line : existing.line); if (existingLine === c.line) { if ((existing.body || "").trim() !== body.trim()) { try { @@ -1429,7 +1544,7 @@ jobs: for (const [fp, existing] of existingByFp) { if (matchedFps.has(fp)) continue; if (humanRepliedIds.has(existing.id)) { stats.sticky++; continue; } - const line = existing.line == null ? existing.original_line : existing.line; + const line = parseLine(existing.line == null ? existing.original_line : existing.line); // No incremental data at all (first review, force-push, unreachable prior // SHA) means there is no way to tell changed from unchanged, so keep the // comment rather than guessing it was fixed. @@ -1507,7 +1622,11 @@ jobs: shell: bash env: COMMAND: ${{ steps.trigger.outputs.command }} + DENIED: ${{ steps.trigger.outputs.denied }} REVIEW_URL: ${{ steps.post_review.outputs.review_url }} + POST_OUTCOME: ${{ steps.post_review.outcome }} + SETUP_OK: ${{ steps.setup.outputs.ok }} + SETUP_FAILED: ${{ steps.setup.outputs.failed }} BEDROCK_OUTCOME: ${{ steps.bedrock.outcome }} ADJUDICATE_OUTCOME: ${{ steps.adjudicate.outcome }} ADJUDICATION_NOTE: ${{ steps.adjudicate.outputs.note }} @@ -1519,12 +1638,23 @@ jobs: echo case "${COMMAND:-review}" in ignore) - echo "No action: the comment did not contain the trigger phrase." ;; + if [ -n "${DENIED:-}" ]; then + echo "No action: the commenter has no write access (author_association: ${DENIED})." + else + echo "No action: the comment did not contain the trigger phrase." + fi ;; dismiss) echo "Dismissed every finding this reviewer posted." ;; *) if [ "${PROCEED:-true}" != "true" ]; then echo "Skipped: ${SKIP_REASON:-not eligible}." + elif [ "${SETUP_OK:-true}" != "true" ]; then + echo ":warning: Setup failed (${SETUP_FAILED:-unknown}); no review this run." + echo + echo "This is advisory and stays green. An OIDC \`AccessDenied\` almost always" + echo "means this repo is not in the reviewer role's trust policy." + elif [ "${POST_OUTCOME:-}" = "failure" ]; then + echo ":warning: The review ran but posting it failed. Check the \"Post Bedrock review\" step logs." elif [ "$BEDROCK_OUTCOME" = "success" ] && [ -n "${REVIEW_URL:-}" ]; then echo "Review posted: $REVIEW_URL" if [ "${ADJUDICATE_OUTCOME:-skipped}" = "failure" ]; then diff --git a/README.md b/README.md index 7429496..7ccf565 100644 --- a/README.md +++ b/README.md @@ -292,6 +292,13 @@ Keeping the `issue_comment` and `pull_request_review_comment` triggers enables: | `@bedrock-review` | Re-review the current HEAD, even if it was already reviewed or the PR is a draft. | | `@bedrock-review dismiss` | Delete every finding this reviewer posted. Resolved threads are left as the permanent record. | +**Both commands require write access.** Anyone can comment on a PR, and these are +privileged: dismiss deletes findings, and on-demand spends against your Bedrock role while +deliberately bypassing the debounce and the already-reviewed gate. The reviewer checks the +comment's `author_association` and acts only for `OWNER`, `MEMBER`, or `COLLABORATOR`. +Anything else is ignored with a `::notice::` naming the association, so a denied request is +visible in the run log rather than silent. + ### Make it sharper per repo Drop a `.github/review-guide.md` with your project's actual blocker and architecture @@ -322,7 +329,7 @@ it. Concrete rules that point at real defects work; style preferences do not. | `min_confidence` | No | `medium` | Drop findings below this self-reported confidence (`low`/`medium`/`high`). A finding with no confidence field counts as `medium`, so an omitted field never silently drops it. | | `inline_min_severity` | No | `suggestion` | Minimum severity that opens an inline thread. Below the floor goes under "Minor notes". | | `max_inline_comments` | No | `10` | Cap on inline threads per run; overflow is listed in the body. | -| `max_files` | No | `50` | Skip PRs touching more files than this; `0` disables. A PR far over the cap blows the diff budget and yields shallow findings. | +| `max_files` | No | `50` | Skip PRs touching more files than this; `0` disables. A PR far over the cap blows the diff budget and yields shallow findings. Counted from the PR's exact `changed_files`, so the cap holds above 100. | | `review_guide_path` | No | `.github/review-guide.md` | Project rules, fed to both passes. A missing file is not an error. | | `trigger_phrase` | No | `@bedrock-review` | On-demand review phrase. Must match the phrase in the caller's `if`. | | `pr_number` | No | auto | Resolved from the triggering event; pass it only for `workflow_dispatch`. | @@ -332,6 +339,11 @@ it. Concrete rules that point at real defects work; style preferences do not. Dependabot PRs are skipped: they run without access to repo or org secrets, so the role assumption cannot succeed, and a no-op beats a red check. +**The advisory check never goes red.** Every step from the OIDC role assumption through +posting is fail-soft, so a misconfiguration degrades to "no review" instead of a red X on +the PR. The run summary names what failed. An OIDC `AccessDenied` almost always means the +repo is not yet in the reviewer role's trust policy. + ### Outputs | Output | Description | @@ -340,9 +352,15 @@ assumption cannot succeed, and a no-op beats a red check. ### Tests -The reconcile logic (which comments to post, move, keep, or delete) is unit-tested. -`tests/reconcile.test.mjs` extracts the workflow's `PURE LOGIC` region from the YAML at -test time and runs it, so there is no second copy to drift out of sync: +The logic that decides what gets posted is unit-tested. Each suite extracts the code under +test out of the workflow YAML at test time, so the workflow file is the only copy and there +is nothing to drift out of sync: + +| Suite | Covers | +|---|---| +| `tests/reconcile.test.mjs` | The `PURE LOGIC` region: comment identity, dismissal and variant matching, the confidence and severity floors, sticky threads, marker ownership, thread preservation. Plus workflow-shape guards (concurrency, fail-soft, exact file count). | +| `tests/trigger.test.mjs` | The `Classify trigger` bash, including the author-association gate and that a hostile comment body cannot reach the shell. | +| `tests/adjudication.test.mjs` | The verdict applier: keep, drop, rewrite, re-grade, and fail-open on unparseable output. | ```bash node --test tests/ diff --git a/tests/adjudication.test.mjs b/tests/adjudication.test.mjs new file mode 100644 index 0000000..ca8b222 --- /dev/null +++ b/tests/adjudication.test.mjs @@ -0,0 +1,217 @@ +// Unit tests for the adjudication verdict-application script in +// bedrock-pr-review.yml. +// +// This script decides which candidate findings survive verification, so a bug +// here silently loses real findings. The Python is extracted verbatim from the +// workflow at test time and run against fixtures, so the workflow file is the +// only copy. + +import { readFileSync, writeFileSync, mkdtempSync, rmSync } from "node:fs"; +import { execFileSync } from "node:child_process"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { strict as assert } from "node:assert"; +import test from "node:test"; + +const WORKFLOW = new URL("../.github/workflows/bedrock-pr-review.yml", import.meta.url); +const source = readFileSync(WORKFLOW, "utf8"); + +// The "Adjudicate findings" step has two heredoc'd Python blocks: the prompt +// builder, then the verdict applier. Take the one that applies verdicts. +function extractApplier() { + const step = source.slice(source.indexOf("- name: Adjudicate findings")); + const blocks = [...step.matchAll(/<<'PY'\n([\s\S]*?)\n\s*PY\n/g)].map((m) => m[1]); + // Both blocks mention "verdicts" (the builder describes the response format), + // so key on something only the applier has. + const applier = blocks.find((b) => b.includes("by_id") && b.includes("adj_path")); + if (!applier) throw new Error("verdict applier block not found in bedrock-pr-review.yml"); + const indent = applier.match(/^\s*/)[0].length; + return applier.split("\n").map((l) => l.slice(indent)).join("\n"); +} + +const APPLIER = extractApplier(); + +function apply(review, adjudicatorOutput) { + const dir = mkdtempSync(join(tmpdir(), "adj-")); + const reviewPath = join(dir, "review.json"); + const adjPath = join(dir, "adj.md"); + const scriptPath = join(dir, "apply.py"); + try { + writeFileSync(reviewPath, JSON.stringify(review)); + writeFileSync(adjPath, adjudicatorOutput); + writeFileSync(scriptPath, APPLIER); + const stdout = execFileSync("python3", [scriptPath, reviewPath, adjPath], { + env: { ...process.env, RUNNER_TEMP: dir }, + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }); + return { review: JSON.parse(readFileSync(reviewPath, "utf8")), stdout }; + } finally { + rmSync(dir, { recursive: true, force: true }); + } +} + +function applyExpectingFailure(review, adjudicatorOutput) { + try { + apply(review, adjudicatorOutput); + } catch (e) { + return e; + } + throw new Error("expected the applier to exit non-zero"); +} + +const candidate = (id, extra = {}) => ({ + _adj_id: id, path: "src/app.py", line: 40, severity: "suggestion", + confidence: "medium", body: `finding ${id}`, ...extra, +}); + +const verdicts = (list) => + `preamble\nBEGIN_ADJUDICATION_JSON\n${JSON.stringify({ verdicts: list })}\nEND_ADJUDICATION_JSON\ntrailer`; + +// --- keep and drop ------------------------------------------------------------ + +test("a dropped candidate is removed and a kept one survives", () => { + const { review } = apply( + { verdict: "COMMENT", summary: "s", comments: [candidate("c0"), candidate("c1")] }, + verdicts([ + { id: "c0", keep: false, reason: "speculative" }, + { id: "c1", keep: true, reason: "real" }, + ]) + ); + assert.equal(review.comments.length, 1); + assert.equal(review.comments[0].body, "finding c1"); +}); + +test("the drop reason is surfaced as a notice, never dropped silently", () => { + const { stdout } = apply( + { comments: [candidate("c0")] }, + verdicts([{ id: "c0", keep: false, reason: "excerpt shows the check already exists" }]) + ); + assert.match(stdout, /::notice::Adjudicator dropped src\/app\.py:40/); + assert.match(stdout, /excerpt shows the check already exists/); +}); + +test("a candidate the judge did not rule on is kept, not lost", () => { + // A truncated adjudicator response must not silently discard findings. + const { review } = apply( + { comments: [candidate("c0"), candidate("c1")] }, + verdicts([{ id: "c0", keep: true }]) + ); + assert.equal(review.comments.length, 2); +}); + +test("the internal adjudication id never reaches the posted finding", () => { + const { review } = apply({ comments: [candidate("c0")] }, verdicts([{ id: "c0", keep: true }])); + assert.ok(!("_adj_id" in review.comments[0]), "_adj_id must be stripped"); +}); + +// --- rewriting and re-grading (the rescue path) ------------------------------- + +test("a rewritten body replaces the candidate's own wording", () => { + const { review } = apply( + { comments: [candidate("c0", { body: "verify that deposit_rate is right" })] }, + verdicts([{ id: "c0", keep: true, rewritten_body: "deposit_rate is hardcoded to 0.10" }]) + ); + assert.equal(review.comments[0].body, "deposit_rate is hardcoded to 0.10"); +}); + +test("an explicit confidence from the judge overrides the worker's grade", () => { + const { review } = apply( + { comments: [candidate("c0", { confidence: "low" })] }, + verdicts([{ id: "c0", keep: true, confidence: "high" }]) + ); + assert.equal(review.comments[0].confidence, "high"); +}); + +test("the judge can also grade a finding down", () => { + const { review } = apply( + { comments: [candidate("c0", { confidence: "high" })] }, + verdicts([{ id: "c0", keep: true, confidence: "medium" }]) + ); + assert.equal(review.comments[0].confidence, "medium"); +}); + +test("a rescued hedged finding is floored at medium so the confidence floor keeps it", () => { + // This is the whole point of the rescue path: the judge rewrites a hedge into + // an assertion. Leaving the worker's `low` grade would let the default medium + // confidence floor discard exactly the finding adjudication just confirmed. + const { review, stdout } = apply( + { comments: [candidate("c0", { confidence: "low", body: "verify that X" })] }, + verdicts([{ id: "c0", keep: true, rewritten_body: "X returns a stale value" }]) + ); + assert.equal(review.comments[0].confidence, "medium"); + assert.match(stdout, /::notice::Re-graded src\/app\.py:40 to medium/); +}); + +test("the medium floor never demotes a high-confidence rewrite", () => { + const { review } = apply( + { comments: [candidate("c0", { confidence: "high" })] }, + verdicts([{ id: "c0", keep: true, rewritten_body: "asserted" }]) + ); + assert.equal(review.comments[0].confidence, "high"); +}); + +test("a keep with no rewrite and no grade leaves the confidence untouched", () => { + const { review } = apply( + { comments: [candidate("c0", { confidence: "low" })] }, + verdicts([{ id: "c0", keep: true }]) + ); + assert.equal(review.comments[0].confidence, "low", "no rescue happened, so no re-grade"); +}); + +test("a nonsense confidence from the judge is ignored rather than written through", () => { + const { review } = apply( + { comments: [candidate("c0", { confidence: "medium" })] }, + verdicts([{ id: "c0", keep: true, confidence: "extremely" }]) + ); + assert.equal(review.comments[0].confidence, "medium"); +}); + +// --- parsing and fail-open ---------------------------------------------------- + +test("a bare JSON response without the markers is still parsed", () => { + const { review } = apply( + { comments: [candidate("c0")] }, + JSON.stringify({ verdicts: [{ id: "c0", keep: false, reason: "dup" }] }) + ); + assert.equal(review.comments.length, 0); +}); + +test("a fenced JSON response is still parsed", () => { + const { review } = apply( + { comments: [candidate("c0")] }, + "```json\n" + JSON.stringify({ verdicts: [{ id: "c0", keep: false, reason: "dup" }] }) + "\n```" + ); + assert.equal(review.comments.length, 0); +}); + +test("unparseable output exits non-zero so the step's continue-on-error keeps every candidate", () => { + // Fail-open is the contract: a broken adjudicator must cost precision, never + // findings. Exiting non-zero leaves the unadjudicated review.json in place. + applyExpectingFailure({ comments: [candidate("c0")] }, "I could not complete this task."); +}); + +test("an empty verdict list exits non-zero rather than dropping the whole review", () => { + applyExpectingFailure({ comments: [candidate("c0")] }, verdicts([])); +}); + +test("a verdict with no id does not silently match a candidate", () => { + const { review } = apply( + { comments: [candidate("c0")] }, + verdicts([{ keep: false, reason: "no id" }]) + ); + assert.equal(review.comments.length, 1, "an unaddressed candidate is kept"); +}); + +test("dropping every candidate is a legitimate outcome", () => { + const { review, stdout } = apply( + { verdict: "COMMENT", summary: "s", comments: [candidate("c0"), candidate("c1")] }, + verdicts([ + { id: "c0", keep: false, reason: "style" }, + { id: "c1", keep: false, reason: "duplicate" }, + ]) + ); + assert.deepEqual(review.comments, []); + assert.equal(review.summary, "s", "the summary and verdict survive an all-drop"); + assert.match(stdout, /kept 0 of 2/); +}); diff --git a/tests/reconcile.test.mjs b/tests/reconcile.test.mjs index 17a3a61..5b6a9e8 100644 --- a/tests/reconcile.test.mjs +++ b/tests/reconcile.test.mjs @@ -33,7 +33,7 @@ const EXPORTS = [ "VARIANT_SIMILARITY_THRESHOLD", "STICKY_LINE_WINDOW", "normalizeAnchor", "firstSentence", "stripMarkers", "extractFingerprint", "fingerprint", "claimTokens", "jaccard", "bestVariantMatch", "meetsFloor", "lineChanged", "anchorOccurrence", - "assertedFingerprints", "ANY_MARKER_RE", "FINGERPRINT_MARKER_RE", + "assertedFingerprints", "ANY_MARKER_RE", "FINGERPRINT_MARKER_RE", "parseLine", ]; const module_ = await import( @@ -49,7 +49,7 @@ const { VARIANT_LINE_WINDOW, LIVE_VARIANT_LINE_WINDOW, VARIANT_SIMILARITY_THRESHOLD, STICKY_LINE_WINDOW, normalizeAnchor, extractFingerprint, fingerprint, claimTokens, jaccard, bestVariantMatch, meetsFloor, lineChanged, anchorOccurrence, - assertedFingerprints, ANY_MARKER_RE, + assertedFingerprints, ANY_MARKER_RE, parseLine, } = module_; // --- the workflow file itself ------------------------------------------------- @@ -61,6 +61,68 @@ test("workflow file carries no control characters that break YAML", () => { assert.equal(offending, null, `found control character ${JSON.stringify(offending && offending[0])}`); }); +test("comment events dedupe by PR, not by run id", () => { + // Keying comment runs on run_id lets two `@bedrock-review` comments run at + // once. Neither sees the other's posts, both open a thread per finding, and + // the loser is unreachable forever after, because the reconcile pass indexes + // one comment per fingerprint. + const block = source.slice(source.indexOf("concurrency:"), source.indexOf("permissions:")); + assert.ok(!block.includes("github.run_id"), "comment runs must not be isolated by run_id"); + assert.ok( + block.includes("github.event.issue.number"), + "an issue_comment carries the PR number as github.event.issue.number" + ); + assert.ok( + block.includes("github.event.pull_request.number"), + "a pull_request or review-comment event carries it as github.event.pull_request.number" + ); + assert.ok( + block.includes("'push'") && block.includes("'command'"), + "push and comment runs must live in separate groups so a comment cannot cancel a review" + ); +}); + +test("the file-count guard uses the exact count, not the capped one", () => { + // `gh pr view --json files` resolves through GraphQL `files(first: 100)`, so it + // saturates at 100. Any max_files above that would silently never fire: a + // 600-file PR reports 100 and sails through a cap of 150. + const block = source.slice(source.indexOf("file_count="), source.indexOf("too_large=\"false\"")); + assert.ok(block.includes(".changed_files"), "count must come from the REST pulls resource"); + assert.ok( + !/\.files\s*\|\s*length/.test(block), + "must not count entries from gh pr view --json files, which caps at 100" + ); + const viewJson = source.slice(source.indexOf("gh pr view"), source.indexOf("> \"$pr_json\"")); + assert.ok(!viewJson.includes(",files"), "drop the files field so nothing can regress onto it"); +}); + +test("nothing before the model call can turn the advisory check red", () => { + // This reviewer is advisory. A misconfiguration must degrade to "no review", + // never to a red X on the consumer's PR. The likeliest failure is the OIDC + // role: until a repo is in the role's trust policy, assume-role returns + // AccessDenied, and one missing trust entry would red-X every PR in the repo. + const mustBeSoft = [ + "Configure AWS credentials (OIDC)", + "Set up Python", + "Install SDKs", + "Write Bedrock caller", + "Run Bedrock review", + "Adjudicate findings", + "Post Bedrock review", + ]; + for (const name of mustBeSoft) { + const at = source.indexOf(`- name: ${name}`); + assert.ok(at > 0, `step not found: ${name}`); + // Look only as far as the next step, so a later step's flag cannot satisfy this. + const next = source.indexOf("\n - name: ", at + 1); + const block = source.slice(at, next < 0 ? source.length : next); + assert.ok( + block.includes("continue-on-error: true"), + `${name} must be fail-soft, or a failure there reddens the consumer's check` + ); + } +}); + test("tuning constants match the calibrated FinalWalk values", () => { assert.equal(VARIANT_LINE_WINDOW, 10); assert.equal(LIVE_VARIANT_LINE_WINDOW, 40); @@ -364,3 +426,27 @@ test("no findings at all leaves every thread a deletion candidate", () => { // only via the sticky rule, which is tested separately. assert.equal(assertedFingerprints([], new Set(["aaa", "bbb"]), new Set()).size, 0); }); + +// --- line coercion ----------------------------------------------------------- + +test("parseLine accepts a real line number from either JSON type", () => { + // The prompt asks for a number but cannot enforce it. A string line silently + // failed the commentable-line Set lookup, demoting the finding to a summary + // bullet even when its line was in the diff. + assert.equal(parseLine(42), 42); + assert.equal(parseLine("42"), 42); + assert.equal(parseLine(" 42 "), 42); +}); + +test("parseLine rejects anything that is not a usable 1-based line", () => { + for (const bad of [0, -1, 1.5, "", "abc", null, undefined, NaN, {}, []]) { + assert.equal(parseLine(bad), 0, `parseLine(${JSON.stringify(bad)}) must be 0`); + } +}); + +test("parseLine makes the moved-or-not decision type-independent", () => { + // The move path deletes and re-posts, which notifies the author and resets the + // thread. A number-versus-string mismatch would classify an unmoved finding as + // a move on every single run. + assert.equal(parseLine("40") === parseLine(40), true); +}); diff --git a/tests/trigger.test.mjs b/tests/trigger.test.mjs new file mode 100644 index 0000000..5076a74 --- /dev/null +++ b/tests/trigger.test.mjs @@ -0,0 +1,190 @@ +// Unit tests for the trigger classification in bedrock-pr-review.yml. +// +// The `Classify trigger` step decides whether a comment drives the reviewer at +// all, and both commands it can reach are privileged: dismiss deletes findings, +// and on-demand spends against the caller's Bedrock role while bypassing the +// debounce and already-reviewed gates. That makes the author-association gate a +// security boundary, so it is tested rather than reasoned about. +// +// The step's bash is extracted verbatim from the workflow at test time and run +// with a temp GITHUB_OUTPUT, so the workflow file is the only copy. + +import { readFileSync, mkdtempSync, rmSync } from "node:fs"; +import { execFileSync } from "node:child_process"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { strict as assert } from "node:assert"; +import test from "node:test"; + +const WORKFLOW = new URL("../.github/workflows/bedrock-pr-review.yml", import.meta.url); +const source = readFileSync(WORKFLOW, "utf8"); + +// Pull one step's `run:` block scalar out of the YAML and dedent it. +function extractRun(stepName) { + const lines = source.split("\n"); + const stepAt = lines.findIndex((l) => l.trim() === `- name: ${stepName}`); + if (stepAt < 0) throw new Error(`step not found: ${stepName}`); + const runAt = lines.findIndex((l, i) => i > stepAt && /^\s*run:\s*\|\s*$/.test(l)); + if (runAt < 0) throw new Error(`no 'run: |' block in step: ${stepName}`); + const indent = lines[runAt].search(/\S/) + 2; + const body = []; + for (let i = runAt + 1; i < lines.length; i++) { + const line = lines[i]; + if (line.trim() === "") { body.push(""); continue; } + if (line.search(/\S/) < indent) break; + body.push(line.slice(indent)); + } + return body.join("\n"); +} + +const CLASSIFY = extractRun("Classify trigger"); + +// Run the step and return what it wrote to GITHUB_OUTPUT. +function classify(env) { + const dir = mkdtempSync(join(tmpdir(), "trigger-")); + const outputPath = join(dir, "github_output"); + try { + execFileSync("bash", ["-c", CLASSIFY], { + env: { + PATH: process.env.PATH, + GITHUB_OUTPUT: outputPath, + TRIGGER_PHRASE: "@bedrock-review", + ...env, + }, + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }); + const parsed = {}; + for (const line of readFileSync(outputPath, "utf8").split("\n")) { + const eq = line.indexOf("="); + if (eq > 0) parsed[line.slice(0, eq)] = line.slice(eq + 1); + } + return parsed; + } finally { + rmSync(dir, { recursive: true, force: true }); + } +} + +const comment = (body, association, extra = {}) => ({ + EVENT_NAME: "issue_comment", + IS_PR_COMMENT: "true", + COMMENT_BODY: body, + COMMENT_ASSOCIATION: association, + ...extra, +}); + +// --- automatic review --------------------------------------------------------- + +test("a pull_request event is always an automatic review", () => { + const out = classify({ EVENT_NAME: "pull_request" }); + assert.equal(out.command, "review"); + assert.equal(out.eligible, "true"); +}); + +test("a pull_request event needs no comment fields at all", () => { + // github.event.comment.* is absent on pull_request, so every comment-shaped + // env var arrives empty. That must not be read as a denied command. + const out = classify({ + EVENT_NAME: "pull_request", COMMENT_BODY: "", COMMENT_ASSOCIATION: "", IS_PR_COMMENT: "false", + }); + assert.equal(out.command, "review"); + assert.equal(out.denied, ""); +}); + +// --- the author-association gate ---------------------------------------------- + +for (const association of ["OWNER", "MEMBER", "COLLABORATOR"]) { + test(`a ${association} can request an on-demand review`, () => { + const out = classify(comment("please look again @bedrock-review", association)); + assert.equal(out.command, "on-demand"); + assert.equal(out.eligible, "true"); + assert.equal(out.denied, ""); + }); + + test(`a ${association} can dismiss`, () => { + const out = classify(comment("@bedrock-review dismiss", association)); + assert.equal(out.command, "dismiss"); + assert.equal(out.eligible, "false", "dismiss must not fall through into a review"); + }); +} + +for (const association of ["NONE", "CONTRIBUTOR", "FIRST_TIME_CONTRIBUTOR", "FIRST_TIMER", "MANNEQUIN", ""]) { + test(`a commenter with association "${association}" cannot request a review`, () => { + // Anyone can comment on a PR. Without this gate a drive-by commenter could + // drive unbounded Bedrock spend against the repo owner's AWS account. + const out = classify(comment("@bedrock-review", association)); + assert.equal(out.command, "ignore"); + assert.equal(out.eligible, "false"); + assert.equal(out.denied, association || "NONE"); + }); + + test(`a commenter with association "${association}" cannot dismiss`, () => { + // And could otherwise delete every finding on every open PR. + const out = classify(comment("@bedrock-review dismiss", association)); + assert.equal(out.command, "ignore"); + assert.equal(out.denied, association || "NONE"); + }); +} + +test("a missing association is treated as no access, not as owner", () => { + const out = classify({ + EVENT_NAME: "issue_comment", IS_PR_COMMENT: "true", COMMENT_BODY: "@bedrock-review dismiss", + }); + assert.equal(out.command, "ignore"); + assert.equal(out.denied, "NONE"); +}); + +// --- phrase matching ---------------------------------------------------------- + +test("a comment without the trigger phrase is ignored, and is not a denial", () => { + const out = classify(comment("looks good to me", "OWNER")); + assert.equal(out.command, "ignore"); + assert.equal(out.denied, "", "no phrase is not an access failure and must not be reported as one"); +}); + +test("dismiss wins over review when both readings are possible", () => { + const out = classify(comment("@bedrock-review dismiss please", "OWNER")); + assert.equal(out.command, "dismiss"); +}); + +test("an issue_comment on a plain issue is ignored", () => { + // issue_comment fires for issues as well as PRs; there is no PR to review. + const out = classify(comment("@bedrock-review", "OWNER", { IS_PR_COMMENT: "false" })); + assert.equal(out.command, "ignore"); +}); + +test("a review comment on the diff is eligible without the issue.pull_request marker", () => { + // pull_request_review_comment has no github.event.issue, so IS_PR_COMMENT is + // false there and must not be consulted. + const out = classify({ + EVENT_NAME: "pull_request_review_comment", + IS_PR_COMMENT: "false", + COMMENT_BODY: "@bedrock-review", + COMMENT_ASSOCIATION: "MEMBER", + }); + assert.equal(out.command, "on-demand"); +}); + +test("an empty trigger phrase disables the commands rather than matching everything", () => { + const out = classify({ ...comment("anything at all", "OWNER"), TRIGGER_PHRASE: "" }); + assert.equal(out.command, "ignore"); +}); + +test("the phrase is matched literally, not as a pattern", () => { + // grep -F, so a phrase containing regex metacharacters still matches itself + // and nothing else. + const withMeta = { ...comment("ping @bedrock.review now", "OWNER"), TRIGGER_PHRASE: "@bedrock.review" }; + assert.equal(classify(withMeta).command, "on-demand"); + const literal = { ...comment("ping @bedrockXreview now", "OWNER"), TRIGGER_PHRASE: "@bedrock.review" }; + assert.equal(classify(literal).command, "ignore", "'.' must not match an arbitrary character"); +}); + +test("a comment body cannot inject shell through the classifier", () => { + // The body is a hostile string: it reaches the step through env, never + // interpolated into the script. If it were interpolated, this would run + // `touch` and the classifier would report something other than a clean ignore. + const hostile = '"; touch /tmp/bedrock-review-pwned; echo "'; + const out = classify(comment(hostile, "OWNER")); + assert.equal(out.command, "ignore"); + assert.equal(out.eligible, "false"); +});