diff --git a/.github/workflows/biggiepockets-review.yml b/.github/workflows/biggiepockets-review.yml index 15a44fd..c50f1ee 100644 --- a/.github/workflows/biggiepockets-review.yml +++ b/.github/workflows/biggiepockets-review.yml @@ -46,6 +46,17 @@ on: required: false default: main type: string + dry_run: + description: >- + Run the review without submitting it. The verdict is uploaded as a + `review-verdict-pr` artifact instead of being posted, which is what the + prompt evals in the registry's evals/ use to score the prompts against pull + requests whose correct outcome is already known. Everything upstream of the + submit step is unchanged on purpose: an eval that runs a different pipeline + measures a different pipeline. + required: false + default: false + type: boolean jobs: # Codex and Claude intentionally run in separate jobs. GitHub can only re-run a @@ -319,6 +330,18 @@ jobs: # Stage 2: Claude verifies Codex's findings, reviews independently, and decides. # This is the only Claude pass, running whichever arm's prompt was assigned to # this PR. Its verdict is the review. + # Allocated here rather than inside the Datadog step below, because the eval + # metrics the prompt evals attach to this review have to join on the same span ids + # this trace is submitted under, and that job runs after this one. + - name: Allocate LLM Obs trace ids + run: | + { + echo "TRACE_ID=$(openssl rand -hex 16)" + echo "ROOT_SPAN_ID=$(openssl rand -hex 8)" + echo "CODEX_SPAN_ID=$(openssl rand -hex 8)" + echo "CLAUDE_SPAN_ID=$(openssl rand -hex 8)" + } >> "$GITHUB_ENV" + - name: Record Claude start time run: echo "CLAUDE_START_NS=$(date +%s%N)" >> "$GITHUB_ENV" @@ -341,7 +364,53 @@ jobs: # Submit the review under the arm this PR was assigned. The arm is not disclosed # in the comment: a reviewer who knows which prompt wrote a summary can't rate it # blind, and the whole point of the split is an unbiased read on the two prompts. + # Evals path: keep the verdict as an artifact rather than posting it. VERDICT is + # still exported so the Datadog reporting below records a dry run the same way it + # records a real one. + - name: Record verdict without submitting + if: ${{ inputs.dry_run }} + run: | + if [ ! -f verdict.json ]; then + echo "::error::Claude review run did not produce verdict.json" >&2 + exit 1 + fi + echo "VERDICT=$(jq -r '.verdict' verdict.json)" >> "$GITHUB_ENV" + echo "Dry run: verdict is $(jq -r '.verdict' verdict.json), not submitting" + + # What the eval run needs to publish its result against this review: the prompt + # that produced it, and the span to hang the eval metric on. ml_app matches the + # trace submission below so the metric lands on the same application. + jq -n \ + --arg pr "$PR" \ + --arg ml_app "biggiepockets-review" \ + --arg arm "${{ steps.resolve.outputs.assigned_arm }}" \ + --arg prompt_name "${{ steps.resolve.outputs.arm_prompt_name }}" \ + --arg prompt_version "${{ steps.resolve.outputs.arm_prompt_version }}" \ + --arg registry_ref "${{ inputs.registry_ref }}" \ + --arg registry_sha "${{ needs.codex.outputs.registry_sha }}" \ + --arg trace_id "$TRACE_ID" \ + --arg claude_span_id "$CLAUDE_SPAN_ID" \ + '{pr: $pr, ml_app: $ml_app, arm: $arm, prompt_name: $prompt_name, + prompt_version: $prompt_version, registry_ref: $registry_ref, + registry_sha: $registry_sha, trace_id: $trace_id, + claude_span_id: $claude_span_id}' > review-meta.json + + # Uploaded even on failure: a missing or malformed verdict is itself an eval + # result, and the scorer treats "no verdict" as a failed case rather than a pass. + - name: Upload verdict for eval scoring + if: ${{ always() && inputs.dry_run }} + uses: actions/upload-artifact@v4 + with: + name: review-verdict-pr${{ inputs.pr }} + path: | + verdict.json + review-meta.json + if-no-files-found: warn + retention-days: 7 + overwrite: true + - name: Submit review as BiggiePockets + if: ${{ !inputs.dry_run }} id: submit env: # PAT authenticating as the BiggiePockets service account. A bot cannot @@ -482,10 +551,10 @@ jobs: run_id="$GITHUB_REPOSITORY-pr$PR-${{ github.run_id }}" - trace_id=$(openssl rand -hex 16) - root_span_id=$(openssl rand -hex 8) - codex_span_id=$(openssl rand -hex 8) - claude_span_id=$(openssl rand -hex 8) + trace_id="$TRACE_ID" + root_span_id="$ROOT_SPAN_ID" + codex_span_id="$CODEX_SPAN_ID" + claude_span_id="$CLAUDE_SPAN_ID" spans=$(jq -n \ --arg trace_id "$trace_id" \ diff --git a/evals/README.md b/evals/README.md new file mode 100644 index 0000000..4add549 --- /dev/null +++ b/evals/README.md @@ -0,0 +1,55 @@ +# Review prompt evals + +A regression set for the BiggiePockets review prompts in `prompts/`. Each case is a real +pull request whose correct review outcome is known, usually because the review that +actually ran got it wrong and the defect was found later. Running the set answers one +question: would today's prompts catch it? + +The runner lives in the repository whose pull requests the cases point at +(`.github/workflows/eval-review-prompts.yml` in `BiggerPockets/biggerpockets`), because +the reusable review workflow reviews the caller's own repo. It calls the same review +workflow that posts real reviews, with `dry_run: true` so nothing is submitted, and +scores each resulting `verdict.json` with `evals/score.sh`. + +Run it against a prompt branch before merging a prompt change: + +``` +gh workflow run eval-review-prompts.yml --repo BiggerPockets/biggerpockets \ + -f registry_ref= +``` + +Every case costs one Codex pass plus one Claude pass, so the set is run on demand rather +than on every prompt push. + +## Adding a case + +Write `evals/cases/.json`: + +```json +{ + "id": "short-kebab-case-id", + "pr": 12345, + "why": "What the diff does, what is wrong with it, and what the real review did.", + "expect_verdict": "request_changes", + "must_match": [["pull-requests", "permission scope"], ["403", "permission"]], + "notes": "Anything a later reader needs to judge whether the assertions are fair." +} +``` + +`must_match` rows are ANDed and the entries within a row are ORed, matched +case-insensitively against the review summary. The point of a row is to separate "found +this bug" from "found some other bug and requested changes anyway" — a case whose only +assertion is the verdict proves very little on its own. Matching cannot judge whether the +reasoning was sound, so keep rows broad enough to survive different phrasing and put +anything a reader needs to re-judge the case in `notes`. + +A case needs a stable pull request to point at. A merged PR works: the runner diffs +`refs/pull//head` against the merge base, so the diff stays what it was. Two things to +watch: + +- Do not point a case at a PR whose discussion contains the answer. The prompts weigh PR + comments, so a thread that explains the defect turns the case into a reading exercise. + When the original PR has been annotated after the fact, open a replay PR at the original + head and close it, and point the case at the replay. +- Keep at least one case whose expected verdict is `approve`. Without one, a prompt that + requests changes on everything scores perfectly. diff --git a/evals/cases/review-app-sweeper-permission-fix.json b/evals/cases/review-app-sweeper-permission-fix.json new file mode 100644 index 0000000..f4bc0fc --- /dev/null +++ b/evals/cases/review-app-sweeper-permission-fix.json @@ -0,0 +1,8 @@ +{ + "id": "review-app-sweeper-permission-fix", + "pr": 30502, + "why": "The fix for the case above: adds `pull-requests: read` and captures the HTTP status so a non-200 is surfaced as a warning annotation instead of being swallowed. A correct review approves this. It is the negative case that stops a prompt from scoring well by requesting changes on everything.", + "expect_verdict": "approve", + "must_match": [], + "notes": "Scored on verdict alone. Kept deliberately: an eval set of defect cases only rewards a reviewer that never approves." +} diff --git a/evals/cases/review-app-sweeper-permission-scope.json b/evals/cases/review-app-sweeper-permission-scope.json new file mode 100644 index 0000000..e3ca07e --- /dev/null +++ b/evals/cases/review-app-sweeper-permission-scope.json @@ -0,0 +1,28 @@ +{ + "id": "review-app-sweeper-permission-scope", + "pr": 30503, + "why": "Replay of BiggerPockets/biggerpockets#30384 at its original head. The workflow it adds declares only `issues: read`, but its /cwa check reads comments on a PR-backed issue, which also needs `pull-requests: read`. The request 403s, `curl -sf` swallows it into an empty string, and the fail-closed guard keeps every app, so the sweeper can never expire anything. The real review approved this and recorded the scope as correctly declared.", + "expect_verdict": "request_changes", + "must_match": [ + [ + "pull-requests", + "pull_requests", + "pull request read", + "permission scope", + "too narrow" + ], + [ + "403", + "permission", + "scope" + ], + [ + "expire", + "sweep", + "never", + "no-op", + "skip" + ] + ], + "notes": "The blocking finding must be the permission scope. A review that requests changes only over the missing pagination on the comments fetch, or only over the swallowed DELETE status, has not found this bug \u2014 the must_match rows are what separate those." +} diff --git a/evals/publish.sh b/evals/publish.sh new file mode 100755 index 0000000..979e3e1 --- /dev/null +++ b/evals/publish.sh @@ -0,0 +1,130 @@ +#!/usr/bin/env bash +# publish.sh — publish one scored eval case to Datadog. +# +# Usage: DD_API_KEY=... evals/publish.sh <0|1> +# the final argument is the score: 1 when the case passed, 0 when it failed. +# +# Publishes the result twice, on purpose, because the two answer different questions: +# +# 1. An LLM Obs evaluation metric joined to the review's Claude span. This puts the +# case result on the same trace as the review that produced it, so a case can be +# read next to the summary it scored and grouped by prompt_version alongside every +# other review the pipeline reports. +# 2. A custom metric series, biggiepockets.review_eval.case_passed. This is what a +# dashboard or a monitor can query — evaluation metrics are not metrics and cannot +# be alerted on. +# +# Failures warn rather than fail the run: a publishing outage should not turn a passing +# eval set red. Each request's HTTP status and body are printed, because a silently +# dropped submission looks exactly like a submission nobody made. +# +# The span join needs span_id in DECIMAL (trace_id may be decimal or 32-char hex), while +# the review workflow allocates both as hex. Converting is the whole reason this script +# is not two inline curls. + +set -euo pipefail + +CASE_FILE="${1:?usage: publish.sh <0|1>}" +VERDICT_FILE="${2:?missing verdict.json}" +META_FILE="${3:?missing review-meta.json}" +PASSED="${4:?missing score (0|1)}" + +DD_SITE="${DD_SITE:-datadoghq.com}" + +if [ -z "${DD_API_KEY:-}" ]; then + echo "Skipping Datadog publish: DD_API_KEY not set" + exit 0 +fi + +CASE_ID=$(jq -r '.id' "$CASE_FILE") +EXPECTED=$(jq -r '.expect_verdict' "$CASE_FILE") +ACTUAL=$(jq -r '.verdict // "none"' "$VERDICT_FILE") + +# A review that died before writing review-meta.json has no span to join to. The custom +# metric still goes out — a case that could not run is a result worth graphing — but say +# so rather than posting an eval metric with an invented join key. +if [ -f "$META_FILE" ]; then + ML_APP=$(jq -r '.ml_app' "$META_FILE") + ARM=$(jq -r '.arm // "unknown"' "$META_FILE") + PROMPT_NAME=$(jq -r '.prompt_name // "unknown"' "$META_FILE") + PROMPT_VERSION=$(jq -r '.prompt_version // "unknown"' "$META_FILE") + REGISTRY_REF=$(jq -r '.registry_ref // "unknown"' "$META_FILE") + PR=$(jq -r '.pr' "$META_FILE") + TRACE_HEX=$(jq -r '.trace_id // empty' "$META_FILE") + SPAN_HEX=$(jq -r '.claude_span_id // empty' "$META_FILE") +else + echo "::warning::$CASE_ID has no review-meta.json; publishing the metric without a span join" + ML_APP="biggiepockets-review" + ARM="unknown"; PROMPT_NAME="unknown"; PROMPT_VERSION="unknown" + REGISTRY_REF="${REGISTRY_REF:-unknown}" + PR=$(jq -r '.pr' "$CASE_FILE") + TRACE_HEX=""; SPAN_HEX="" +fi + +TAGS=( + "case:$CASE_ID" + "pr:$PR" + "expected_verdict:$EXPECTED" + "actual_verdict:$ACTUAL" + "arm:$ARM" + "prompt_name:$PROMPT_NAME" + "prompt_version:$PROMPT_VERSION" + "registry_ref:$REGISTRY_REF" +) +tags_json=$(printf '%s\n' "${TAGS[@]}" | jq -R . | jq -s .) + +post() { # post + local url="$1" payload="$2" what="$3" code + code=$(curl -sS -o /tmp/dd-publish-body.txt -w '%{http_code}' \ + -X POST "$url" \ + -H "Content-Type: application/json" \ + -H "DD-API-KEY: $DD_API_KEY" \ + -d "$payload") || code="000" + if [ "$code" = "000" ] || [ "$code" -ge 300 ] 2>/dev/null; then + echo "::warning::$what failed for $CASE_ID (HTTP $code): $(cat /tmp/dd-publish-body.txt 2>/dev/null)" + return 0 + fi + echo "$what accepted for $CASE_ID (HTTP $code)" +} + +now_ms=$(( $(date +%s) * 1000 )) +passed_bool=false +[ "$PASSED" = "1" ] && passed_bool=true + +if [ -n "$TRACE_HEX" ] && [ -n "$SPAN_HEX" ]; then + # 64-bit span ids exceed what bash arithmetic can hold, so convert in python. + span_dec=$(python3 -c "import sys; print(int(sys.argv[1], 16))" "$SPAN_HEX") + eval_payload=$(jq -n \ + --arg ml_app "$ML_APP" \ + --arg trace_id "$TRACE_HEX" \ + --arg span_id "$span_dec" \ + --arg label_pass "review_eval.case_passed" \ + --arg label_case "review_eval.case" \ + --arg case_id "$CASE_ID" \ + --argjson passed "$passed_bool" \ + --argjson ts "$now_ms" \ + --argjson tags "$tags_json" \ + '{data: {type: "evaluation_metric", attributes: {metrics: [ + {eval_scope: "span", + join_on: {span: {span_id: $span_id, trace_id: $trace_id}}, + ml_app: $ml_app, timestamp_ms: $ts, + metric_type: "boolean", label: $label_pass, + boolean_value: $passed, tags: $tags}, + {eval_scope: "span", + join_on: {span: {span_id: $span_id, trace_id: $trace_id}}, + ml_app: $ml_app, timestamp_ms: $ts, + metric_type: "categorical", label: $label_case, + categorical_value: $case_id, tags: $tags} + ]}}}') + post "https://api.$DD_SITE/api/intake/llm-obs/v2/eval-metric" "$eval_payload" "LLM Obs eval metric" +fi + +series_payload=$(jq -n \ + --argjson value "$PASSED" \ + --argjson ts "$(date +%s)" \ + --argjson tags "$tags_json" \ + '{series: [{metric: "biggiepockets.review_eval.case_passed", + type: 3, + points: [{timestamp: $ts, value: $value}], + tags: $tags}]}') +post "https://api.$DD_SITE/api/v2/series" "$series_payload" "Custom metric" diff --git a/evals/score.sh b/evals/score.sh new file mode 100755 index 0000000..be372b3 --- /dev/null +++ b/evals/score.sh @@ -0,0 +1,78 @@ +#!/usr/bin/env bash +# score.sh — score one review verdict against one eval case. +# +# Usage: evals/score.sh +# +# A case asserts two things about a review of a known pull request: +# expect_verdict the verdict the review must reach ("approve" | "request_changes") +# must_match rows of acceptable phrasings; the summary must match at least one +# entry in EVERY row (rows are ANDed, entries within a row are ORed) +# +# Matching is case-insensitive substring matching, which is deliberately crude: it can +# only tell that a review talked about the right thing, never that its reasoning was +# sound. Keep the entries broad enough that a correct review phrased differently still +# passes, and narrow enough that a review which found a DIFFERENT problem fails. When a +# case cannot be expressed that way, score it on the verdict alone (empty must_match) +# and record why in the case's "notes". +# +# Exits 0 when the case passes, 1 when it fails, 2 on bad input. Prints one line per +# assertion so a failure says which one and what the review said instead. + +set -euo pipefail + +CASE_FILE="${1:?usage: score.sh }" +VERDICT_FILE="${2:?usage: score.sh }" + +for f in "$CASE_FILE" "$VERDICT_FILE"; do + [ -f "$f" ] || { echo "score: $f not found" >&2; exit 2; } +done + +CASE_ID=$(jq -r '.id' "$CASE_FILE") +PR=$(jq -r '.pr' "$CASE_FILE") +EXPECT=$(jq -r '.expect_verdict' "$CASE_FILE") + +# A review that crashed leaves no verdict. Treat that as a case failure rather than a +# pass, so an eval run can't come back green because the pipeline never produced output. +GOT=$(jq -r '.verdict // empty' "$VERDICT_FILE") +SUMMARY=$(jq -r '.summary // empty' "$VERDICT_FILE" | tr '[:upper:]' '[:lower:]') +if [ -z "$GOT" ]; then + echo "FAIL $CASE_ID (pr #$PR): review produced no verdict" + exit 1 +fi + +failed=0 + +if [ "$GOT" = "$EXPECT" ]; then + echo " pass verdict is $GOT" +else + echo " FAIL verdict is $GOT, expected $EXPECT" + failed=1 +fi + +rows=$(jq -r '.must_match | length' "$CASE_FILE") +for i in $(seq 0 $((rows - 1))); do + [ "$rows" -eq 0 ] && break + matched="" + while IFS= read -r phrase; do + lower=$(printf '%s' "$phrase" | tr '[:upper:]' '[:lower:]') + case "$SUMMARY" in *"$lower"*) matched="$phrase"; break;; esac + done < <(jq -r --argjson i "$i" '.must_match[$i][]' "$CASE_FILE") + if [ -n "$matched" ]; then + echo " pass summary mentions '$matched'" + else + echo " FAIL summary mentions none of: $(jq -r --argjson i "$i" '.must_match[$i] | join(", ")' "$CASE_FILE")" + failed=1 + fi +done + +if [ "$failed" -eq 0 ]; then + echo "PASS $CASE_ID (pr #$PR)" +else + echo "FAIL $CASE_ID (pr #$PR)" + # Echo the summary on a failure only — it is the evidence for why the case failed, and + # printing it on every case would bury the result lines. + echo "--- review summary ---" + jq -r '.summary // "(none)"' "$VERDICT_FILE" + echo "----------------------" +fi +exit "$failed"