diff --git a/.github/workflows/bedrock-pr-review.yml b/.github/workflows/bedrock-pr-review.yml index 2ea6079..0fff4e8 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,92 @@ 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 }} + # 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.pull_request.number + || github.event.issue.number + || inputs.pr_number + }}-${{ github.event_name == 'pull_request' && 'push' || 'command' }} cancel-in-progress: true permissions: @@ -114,25 +207,136 @@ 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 }} + 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"; denied_association="" + 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}"; 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 "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" + + # --------------------------------------------------------------- + # 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,14 +344,16 @@ 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" @@ -160,6 +366,20 @@ 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")" + # 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. + 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 +400,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 +461,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,78 +517,93 @@ 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). # --------------------------------------------------------------- + # 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 # --------------------------------------------------------------- - # 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 + id: 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 +613,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 +624,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 +634,112 @@ 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" + + # --------------------------------------------------------------- + # 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' && steps.setup.outputs.ok == '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,87 +752,816 @@ 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": "", "confidence": "high"}]} + END_ADJUDICATION_JSON + + `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, ""] + 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 + 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 + 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 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 }} 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; }); + } + + // 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, + // 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 { - 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}`); + 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) { + 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; } - } catch (e) { core.warning(`existing comments: ${e.message}`); } + const lines = fileLines(c.path); + 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({ + path: c.path, lineAnchor: lineAnchor, anchor_text: c.anchor_text, + body: c.body, occurrence: occurrence, + }), + })); + } - 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); } + // 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 { + 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) { + // 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 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); + } + + // 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 = parseLine(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++; + } } - 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. + for (const [fp, existing] of existingByFp) { + if (matchedFps.has(fp)) continue; + if (humanRepliedIds.has(existing.id)) { stats.sticky++; continue; } + 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. + 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 +1575,97 @@ 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 }} + 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 }} 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) + 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 + 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..7ccf565 100644 --- a/README.md +++ b/README.md @@ -243,6 +243,137 @@ 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. | + +**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 +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. 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`. | +| `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. + +**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 | +|---|---| +| `review_url` | URL of the persistent review summary comment. | + +### Tests + +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/ +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/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 new file mode 100644 index 0000000..5b6a9e8 --- /dev/null +++ b/tests/reconcile.test.mjs @@ -0,0 +1,452 @@ +// 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", "parseLine", +]; + +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, parseLine, +} = 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("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); + 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); +}); + +// --- 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"); +});