From c0578bf1d7dc08dc91066d023763bf0feb24623e Mon Sep 17 00:00:00 2001 From: Aaron Breckenridge Date: Fri, 28 Aug 2026 11:56:35 -0500 Subject: [PATCH 1/7] Review PRs with a three-auditor panel and an arbitrator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the single synthesizing review pass with a panel. Three auditors — caspar, balthazar, and melchior — each audit the change independently, in separate jobs, running the same auditor prompt, and each writes its own verdict. An arbitrator then weighs all three and issues the one verdict that gets posted. Independence is structural, not asked for in the prompt: a seat runs on its own runner, reads only the stage-1 handoff, and writes only its own verdict to audit-.json. The arbitration job is the first and only place in a run where more than one verdict exists together, so a disagreement between seats means they judged the code differently rather than that one saw the other's answer. The seats run different models (luna, gemini-flash, glm-flash) on identical prompt text, arbitrated by sol. Holding the prompt fixed is what makes a split between seats signal about the change; three runs of one model would agree trivially and leave the arbitrator nothing to weigh. Every stage's model is an OpenRouter slug overrideable by a repository variable, so changing a seat's model never touches a prompt. Arbitration weighs rather than tallies: a blocking finding confirmed in the code outweighs approvals that missed it, one that can be disproved is discarded, and unsupported unanimity is overruled with an explanation. It may confirm or discard what the panel raised but may not introduce a blocking issue no auditor found. A seat that produces no valid verdict is recorded as failed rather than counted as an approval, and arbitration needs a majority of seats to have reported. Requiring only a majority keeps one rate-limited seat from failing a whole review, and is what makes a 1-1 tie reachable — tagged no_majority, since there is no majority to follow or override. Datadog now carries one span per seat plus each seat's verdict, confidence, and model, alongside panel_agreement and arbiter_followed_panel. Those last two are the ones worth watching: near-total unanimity means the extra seats aren't adding information, and an arbitrator that never departs from the majority is a vote counter. Remove the dual-arm A/B apparatus (arms, control_arm, experiment_split_percent, and : bucket assignment). It cannot express "three seats, one prompt", and it had almost no usable data: seven runs, all predating model pinning, on since-rolled prompt versions. Git history keeps it recoverable. Also fix a latent resolver bug the arbitrator prompt exposes — prompt_version ran `cat` over an empty-array expansion, which aborts the resolve for any prompt with no {{@}} includes. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/biggiepockets-review.yml | 602 +++++++++++++----- .github/workflows/validate-prompts.yml | 185 +++--- README.md | 231 ++++--- prompts/claude-synthesize.md | 53 -- prompts/magi-arbitrate.md | 53 ++ ...nthesize-thesis-first.md => magi-audit.md} | 52 +- prompts/registry.json | 44 +- scripts/resolve-prompts.sh | 143 ++--- 8 files changed, 885 insertions(+), 478 deletions(-) delete mode 100644 prompts/claude-synthesize.md create mode 100644 prompts/magi-arbitrate.md rename prompts/{claude-synthesize-thesis-first.md => magi-audit.md} (58%) diff --git a/.github/workflows/biggiepockets-review.yml b/.github/workflows/biggiepockets-review.yml index 4ca96ef..a89810e 100644 --- a/.github/workflows/biggiepockets-review.yml +++ b/.github/workflows/biggiepockets-review.yml @@ -1,8 +1,19 @@ name: BiggiePockets Code Review (reusable) -# Reusable two-stage AI review (Codex first pass, then Claude verifies and -# synthesizes) anchored on the PR's JIRA ticket, with the BiggiePockets service -# account submitting the resulting approve / request-changes decision. +# Reusable AI review panel, anchored on the PR's JIRA ticket, with the BiggiePockets +# service account submitting the resulting approve / request-changes decision. +# +# A review runs in three stages: +# 1. First pass — Codex reads the diff against the ticket and writes leads to +# codex-findings.md. +# 2. Panel — three auditors (caspar, balthazar, melchior) each audit the change +# INDEPENDENTLY, in separate jobs, running the SAME auditor prompt, +# and each writes its own verdict. No auditor can see another's +# verdict: a seat's output is uploaded per seat and downloaded only by +# stage 3. +# 3. Arbitration — one arbitrator reads all three verdicts, verifies the blocking +# findings against the code, and issues the ONE verdict that is posted. +# It is the only stage whose output reaches the pull request. # # Callers own the triggers and gating (e.g. fire on review_requested when # BiggiePockets is the requested reviewer) and pass the PR number as `pr`. @@ -11,23 +22,28 @@ name: BiggiePockets Code Review (reusable) # BIGGIEPOCKETS_PAT, DATADOG_API_KEY (optional; enables review-quality # metrics in Datadog LLM Obs). # -# Prompt versioning: every review-stage prompt lives in this repo's prompts/ -# registry (BiggerPockets/.github) as a versioned template, plus one shared rule -# block per concern under prompts/_shared/ injected via {{@path}}. -# Prompts are resolved by scripts/resolve-prompts.sh into step outputs; a derived -# prompt_version (content hash) identifies each prompt in Datadog LLM Obs. Each LLM -# span also carries its template and variables under meta.input.prompt, which is what -# Datadog Prompt Tracking reads to group runs by prompt and version. +# Models: every stage's model is an OpenRouter slug pinned below and overrideable per +# repository without editing this workflow — set the matching Actions *variable* in the +# calling repo (Settings → Actions → Variables), which the `vars` context below reads. +# An unset or empty variable keeps the pinned default. +# CODEX_MODEL first pass (default openai/gpt-5.6-sol) +# MAGI_MODEL_CASPAR caspar (default openai/gpt-5.6-luna) +# MAGI_MODEL_BALTHAZAR balthazar (default google/gemini-3.7-flash) +# MAGI_MODEL_MELCHIOR melchior (default z-ai/glm-5.3-flash) +# MAGI_MODEL_DEFAULT any seat with no default and no override of its own +# ARBITER_MODEL arbitration (default openai/gpt-5.6-sol) +# The three seats run different models on purpose: the auditor prompt is identical, so +# what a disagreement between seats measures is the models, and three runs of one model +# would mostly agree trivially and leave the arbitrator nothing to weigh. # -# Prompt A/B test (BIG-21717): prompts/registry.json declares the arms, the control -# arm, and what share of pull requests an experiment arm gets. Each review runs ONE -# Claude pass, using the arm that scripts/resolve-prompts.sh assigns to that PR by -# hashing ':' — so the arms are compared BETWEEN pull requests and a review -# never costs a second Claude pass. The assigned arm writes both the posted summary -# and the approve / request-changes decision, which means an experiment prompt does -# affect real outcomes on its share of PRs. Assignment is deterministic, so a re-run -# reviews with the same arm. Set experiment_split_percent to 0 in registry.json to -# route everything to the control arm; every consuming repo reads that one file. +# Prompt versioning: every prompt lives in this repo's prompts/ registry +# (BiggerPockets/.github) as a versioned template, plus one shared rule block per +# concern under prompts/_shared/ injected via {{@path}}. Prompts are resolved by +# scripts/resolve-prompts.sh into step outputs; a derived prompt_version (content hash) +# identifies each prompt in Datadog LLM Obs. Each LLM span also carries its template and +# variables under meta.input.prompt, which is what Datadog Prompt Tracking reads to group +# runs by prompt and version. The panel seats all resolve the SAME auditor prompt, so one +# prompt_name/prompt_version covers all three audit spans. on: workflow_call: inputs: @@ -48,31 +64,31 @@ on: type: string jobs: - # Codex and Claude intentionally run in separate jobs. GitHub can only re-run a - # failed job (not an individual step), so this boundary lets a rate-limited Claude - # run be retried without paying for another Codex pass. + # Stage 1. Gathers every input the panel reads and runs the Codex first pass, then + # uploads the lot as one immutable handoff. Separate from the panel jobs because GitHub + # can only re-run a failed JOB (not an individual step): this boundary lets a + # rate-limited auditor be retried without paying for another Codex pass or re-fetching + # the diff, and guarantees all three auditors judge the exact same commit and context. + # + # Job id stays `codex` so a repo that has this stage as a required status check keeps + # matching it. codex: runs-on: ubuntu-latest outputs: head_sha: ${{ steps.pr_head.outputs.sha }} registry_sha: ${{ steps.registry_head.outputs.sha }} + auditors: ${{ steps.resolve.outputs.auditors }} permissions: contents: read pull-requests: read id-token: write # claude-code-action authenticates via OIDC env: PR: ${{ inputs.pr }} - # Explicit model per stage, threaded into both the actual invocation below and the - # Datadog LLM Obs tags/spans at the bottom of this job. Empty means "let the tool - # use its own default" (today's behavior, unchanged). Set these once you want a - # specific model pinned and reliably visible in telemetry — e.g. when Codex is - # pointed at a non-default model — instead of an implicit, untracked default. - # The Codex stage runs through OpenRouter, so its model must be an OpenRouter - # model slug and must be set explicitly — Codex's own default model name is not - # one OpenRouter accepts. - CODEX_MODEL: "openai/gpt-5.6-sol" + # The first pass runs through OpenRouter, so its model must be an OpenRouter model + # slug and must be set explicitly — Codex's own default model name is not one + # OpenRouter accepts. + CODEX_MODEL: ${{ vars.CODEX_MODEL || 'openai/gpt-5.6-sol' }} CODEX_RESPONSES_ENDPOINT: "https://openrouter.ai/api/v1/responses" - CLAUDE_MODEL: "google/gemini-3.7-flash" steps: - name: Checkout PR head uses: actions/checkout@v4 @@ -80,8 +96,8 @@ jobs: ref: refs/pull/${{ inputs.pr }}/head fetch-depth: 0 - # Pin the Claude job to the exact commit Codex reviewed. This matters when a - # failed Claude job is retried after the PR has received another push. + # Pin every later job to the exact commit the first pass read. This matters when a + # failed audit or arbitration job is retried after the PR has received another push. - name: Record PR head SHA id: pr_head run: echo "sha=$(git rev-parse HEAD)" >> "$GITHUB_OUTPUT" @@ -94,7 +110,7 @@ jobs: # in the CALLER's repo and so is not a valid ref here. # GITHUB_TOKEN is scoped to the caller's repo, so use the BiggiePockets PAT. # persist-credentials is off so the PAT is never left in registry/.git/config where - # the later Codex/Claude steps (which act on untrusted PR diffs and can read files/run + # the later model steps (which act on untrusted PR diffs and can read files/run # Bash) could read it — read-only prompts only need the checkout itself, not the # stored credentials. - name: Checkout prompt registry @@ -184,18 +200,19 @@ jobs: > conversations.json echo "Wrote conversations.json ($(jq '.issue_comments | length' conversations.json) comments, $(jq '.review_comments | length' conversations.json) review comments, $(jq '.reviews | length' conversations.json) reviews)" - # Resolve the review-stage prompts from the registry: the Codex prompt, plus the - # prompt of the single arm this PR is assigned to, each with its shared-rule blocks - # expanded and a content-derived prompt_version. Also emits the assigned arm's key - # and the bucket it was assigned from. + # Resolve the prompts from the registry — the first-pass prompt, the single auditor + # prompt every seat runs, and the arbitrator prompt — each with its shared-rule + # blocks expanded and a content-derived prompt_version. Also emits the panel seats, + # which drive the audit job's fan-out matrix. - name: Resolve review prompts id: resolve env: REGISTRY_DIR: registry run: registry/scripts/resolve-prompts.sh - # Stage 1: Codex reviews the diff against the ticket intent and writes findings to - # codex-findings.md (the `output-file`, which captures Codex's final message). + # Stage 1: Codex reviews the diff against the ticket intent and writes leads to + # codex-findings.md (the `output-file`, which captures Codex's final message). The + # panel treats these as leads to verify, not conclusions to adopt. # # Codex reviews the diff directly here rather than shelling out to the `codex review` # subcommand. `codex review` starts its own in-process app-server that needs both a @@ -207,12 +224,12 @@ jobs: # ("stream disconnected before completion: error sending request for url # http://127.0.0.1.../v1/responses"), because that profile grants filesystem writes but # no network. Either way Codex produced no findings and every review silently degraded to - # a Claude-only pass. Reviewing inline via this step's own `codex exec` model call avoids + # a panel-only pass. Reviewing inline via this step's own `codex exec` model call avoids # the nested subprocess entirely: it needs only to read files, so a read-only sandbox is # sufficient and there is no child process to sandbox. # # continue-on-error keeps a Codex failure (e.g. an OpenAI "Quota exceeded" error) from - # blocking the review: Stage 2 proceeds on its own when codex-findings.md is missing or + # blocking the review: the panel proceeds on its own when codex-findings.md is missing or # empty. The next step still surfaces the failure as a warning so a genuinely broken Codex # (e.g. a bad key) doesn't silently disappear. - name: Record codex start time @@ -231,16 +248,16 @@ jobs: prompt: ${{ steps.resolve.outputs.codex_prompt }} # If Codex failed (it exits nonzero and writes no codex-findings.md — e.g. on a quota - # error), leave an empty findings file so Stage 2 cleanly takes its no-findings path, + # error), leave an empty findings file so the panel cleanly takes its no-findings path, # and record a warning for observability. - name: Note skipped Codex first pass if: steps.codex.outcome == 'failure' run: | - echo "::warning::Codex first-pass review failed (commonly an OpenAI quota error); continuing with a Claude-only review." + echo "::warning::Codex first-pass review failed (commonly an OpenAI quota error); continuing with a panel-only review." : > codex-findings.md - # Persist the handoff even when Codex failed. Claude consumes the same diff, - # ticket/discussion context, and (possibly empty) findings on every retry. + # Persist the handoff even when Codex failed. Every auditor consumes the same diff, + # ticket/discussion context, and (possibly empty) findings, on every retry. - name: Record Codex completion metadata if: always() run: | @@ -252,14 +269,15 @@ jobs: echo "CODEX_START_NS=$CODEX_START_NS" echo "CODEX_END_NS=$codex_end_ns" echo "CODEX_STATUS=${{ steps.codex.outcome }}" + echo "CODEX_MODEL_USED=$CODEX_MODEL" } > review-context.env touch codex-findings.md - - name: Upload Codex review handoff + - name: Upload review handoff if: always() uses: actions/upload-artifact@v4 with: - name: codex-review-${{ github.run_id }} + name: review-context-${{ github.run_id }} path: | pr.diff ticket.json @@ -270,19 +288,35 @@ jobs: retention-days: 7 overwrite: true - # A failed/rate-limited Claude run can be retried with "Re-run failed jobs". The - # successful Codex job is not re-executed; this job downloads its immutable handoff. - claude: + # Stage 2. One job per panel seat, fanned out over the seats the registry declares. + # Separate jobs are what make the audits independent: each runs on its own runner, sees + # only the stage-1 handoff, and writes only its own verdict. Nothing an auditor can read + # contains another auditor's output. + # + # fail-fast is off so one seat's failure doesn't cancel the others mid-audit, and the + # model step is continue-on-error so a rate-limited seat still leaves a recorded status + # for the arbitrator. "Re-run failed jobs" re-runs only the seats that failed. + audit: needs: codex runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + auditor: ${{ fromJSON(needs.codex.outputs.auditors) }} permissions: contents: read pull-requests: read id-token: write # claude-code-action authenticates via OIDC env: PR: ${{ inputs.pr }} - CODEX_MODEL: "openai/gpt-5.6-sol" - CLAUDE_MODEL: "google/gemini-3.7-flash" + AUDITOR: ${{ matrix.auditor }} + # One OpenRouter slug per seat, each overrideable by the matching repository + # variable. MAGI_MODEL_DEFAULT only catches a seat added to registry.json that has + # no entry here, so a new seat runs rather than failing the matrix. + MAGI_MODEL_CASPAR: ${{ vars.MAGI_MODEL_CASPAR || 'openai/gpt-5.6-luna' }} + MAGI_MODEL_BALTHAZAR: ${{ vars.MAGI_MODEL_BALTHAZAR || 'google/gemini-3.7-flash' }} + MAGI_MODEL_MELCHIOR: ${{ vars.MAGI_MODEL_MELCHIOR || 'z-ai/glm-5.3-flash' }} + MAGI_MODEL_DEFAULT: ${{ vars.MAGI_MODEL_DEFAULT || 'google/gemini-3.7-flash' }} steps: - name: Checkout reviewed PR head uses: actions/checkout@v4 @@ -299,10 +333,10 @@ jobs: persist-credentials: false path: registry - - name: Download Codex review handoff + - name: Download review handoff uses: actions/download-artifact@v4 with: - name: codex-review-${{ github.run_id }} + name: review-context-${{ github.run_id }} path: . - name: Restore review context @@ -316,14 +350,31 @@ jobs: REGISTRY_DIR: registry run: registry/scripts/resolve-prompts.sh - # 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. - - name: Record Claude start time - run: echo "CLAUDE_START_NS=$(date +%s%N)" >> "$GITHUB_ENV" + # Look the seat's model up by name rather than hard-coding a model per matrix leg, + # so adding a seat to registry.json needs no change here and an override is a + # repository variable rather than a workflow edit. + - name: Select seat model + id: seat + run: | + var="MAGI_MODEL_$(printf '%s' "$AUDITOR" | tr '[:lower:]-' '[:upper:]_')" + model="${!var:-}" + if [ -z "$model" ]; then model="$MAGI_MODEL_DEFAULT"; fi + if [ -z "$model" ]; then + echo "::error::No model resolved for auditor '$AUDITOR' (set $var or MAGI_MODEL_DEFAULT)" >&2 + exit 1 + fi + echo "model=$model" >> "$GITHUB_OUTPUT" + echo "Auditor $AUDITOR runs $model" - - name: Claude verify and synthesize - id: claude + - name: Record audit start time + run: echo "AUDIT_START_NS=$(date +%s%N)" >> "$GITHUB_ENV" + + # The auditor prompt is identical for every seat — the resolver emits it once and + # substitutes nothing seat-specific — so the three audits differ only by model and + # by the independent judgment of the run. + - name: Audit independently + id: audit + continue-on-error: true uses: anthropics/claude-code-action@v1 with: # The action requires this input for launch validation. ANTHROPIC_AUTH_TOKEN @@ -333,29 +384,198 @@ jobs: # by the claude bot are rejected with "Workflow initiated by non-human # actor: claude (type: Bot). Add bot to allowed_bots list." allowed_bots: 'claude' - claude_args: ${{ env.CLAUDE_MODEL != '' && format('--allowedTools "Read,Grep,Glob,Bash,Write" --model {0}', env.CLAUDE_MODEL) || '--allowedTools "Read,Grep,Glob,Bash,Write"' }} - prompt: ${{ steps.resolve.outputs.arm_prompt }} + claude_args: '--allowedTools "Read,Grep,Glob,Bash,Write" --model ${{ steps.seat.outputs.model }}' + prompt: ${{ steps.resolve.outputs.auditor_prompt }} env: ANTHROPIC_BASE_URL: "https://openrouter.ai/api" ANTHROPIC_AUTH_TOKEN: ${{ secrets.OPENROUTER_API_KEY }} - - name: Record Claude end time + # Name the verdict for its seat before it leaves this job. The arbitrator reads + # audit-.json, and a seat that produced no usable verdict must be visible as + # such rather than absent: the recorded status is what stops a failed seat from + # being mistaken for a silent approval. + - name: Record seat verdict if: always() - run: echo "CLAUDE_END_NS=$(date +%s%N)" >> "$GITHUB_ENV" + run: | + audit_end_ns=$(date +%s%N) + status="ok" + [ "${{ steps.audit.outcome }}" != "success" ] && status="error" + + if [ -f verdict.json ] && jq -e '.verdict == "approve" or .verdict == "request_changes"' verdict.json >/dev/null 2>&1; then + mv verdict.json "audit-$AUDITOR.json" + echo "$AUDITOR: $(jq -r '.verdict' "audit-$AUDITOR.json") (confidence $(jq -r '.confidence // "unstated"' "audit-$AUDITOR.json"), $(jq -r '.blocking_findings // [] | length' "audit-$AUDITOR.json") blocking finding(s))" + else + status="error" + rm -f verdict.json + echo "::warning::Auditor $AUDITOR produced no valid verdict.json; the arbitrator will arbitrate without this seat." + fi + + # Shell-safe seat name so the arbitrator can source every seat's file at once. + key=$(printf '%s' "$AUDITOR" | tr '[:lower:]-' '[:upper:]_') + { + echo "AUDIT_${key}_START_NS=$AUDIT_START_NS" + echo "AUDIT_${key}_END_NS=$audit_end_ns" + echo "AUDIT_${key}_STATUS=$status" + echo "AUDIT_${key}_MODEL=${{ steps.seat.outputs.model }}" + } > "audit-$AUDITOR.env" + + - name: Upload seat verdict + if: always() + uses: actions/upload-artifact@v4 + with: + name: audit-${{ matrix.auditor }}-${{ github.run_id }} + path: | + audit-${{ matrix.auditor }}.json + audit-${{ matrix.auditor }}.env + if-no-files-found: error + retention-days: 7 + overwrite: true + + # Stage 3. Weighs the three verdicts and issues the one that gets posted. + arbitrate: + needs: [codex, audit] + runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: read + id-token: write # claude-code-action authenticates via OIDC + env: + PR: ${{ inputs.pr }} + ARBITER_MODEL: ${{ vars.ARBITER_MODEL || 'openai/gpt-5.6-sol' }} + steps: + - name: Checkout reviewed PR head + uses: actions/checkout@v4 + with: + ref: ${{ needs.codex.outputs.head_sha }} + fetch-depth: 0 + + - name: Checkout prompt registry + uses: actions/checkout@v4 + with: + repository: BiggerPockets/.github + ref: ${{ needs.codex.outputs.registry_sha }} + token: ${{ secrets.BIGGIEPOCKETS_PAT }} + persist-credentials: false + path: registry + + - name: Download review handoff + uses: actions/download-artifact@v4 + with: + name: review-context-${{ github.run_id }} + path: . + + # Every seat's artifact flattened into the working directory. Filenames carry the + # seat name, so merging can't collide, and this is the first and only place in the + # run where more than one auditor's verdict exists together. + - name: Download panel verdicts + uses: actions/download-artifact@v4 + with: + pattern: audit-*-${{ github.run_id }} + merge-multiple: true + path: . + + - name: Restore review context + run: | + cat review-context.env >> "$GITHUB_ENV" + for f in audit-*.env; do [ -f "$f" ] && cat "$f" >> "$GITHUB_ENV"; done + true + + - name: Resolve review prompts + id: resolve + env: + REGISTRY_DIR: registry + run: registry/scripts/resolve-prompts.sh + + # A panel that lost most of its seats isn't a panel. Arbitrating over one verdict + # would quietly turn a three-auditor review into a single-reviewer one, so require + # a majority of seats to have produced a verdict and fail loudly otherwise — the + # failed audit jobs can then be re-run without repeating stage 1. + - name: Check panel quorum + env: + AUDITOR_COUNT: ${{ steps.resolve.outputs.auditor_count }} + run: | + seats=$(ls audit-*.json 2>/dev/null | wc -l | tr -d ' ') + quorum=$(( AUDITOR_COUNT / 2 + 1 )) + echo "PANEL_SEATS_REPORTING=$seats" >> "$GITHUB_ENV" + echo "PANEL_SEATS_DECLARED=$AUDITOR_COUNT" >> "$GITHUB_ENV" + if [ "$seats" -lt "$quorum" ]; then + echo "::error::Only $seats of $AUDITOR_COUNT auditors produced a verdict (quorum $quorum); re-run the failed audit jobs." >&2 + exit 1 + fi + if [ "$seats" -lt "$AUDITOR_COUNT" ]; then + echo "::warning::Arbitrating over $seats of $AUDITOR_COUNT auditors." + fi + for f in audit-*.json; do + echo "$f -> $(jq -r '.verdict' "$f") (confidence $(jq -r '.confidence // "unstated"' "$f"))" + done + + - name: Record arbitration start time + run: echo "ARBITER_START_NS=$(date +%s%N)" >> "$GITHUB_ENV" + + - name: Arbitrate the panel + id: arbiter + uses: anthropics/claude-code-action@v1 + with: + anthropic_api_key: ${{ secrets.OPENROUTER_API_KEY }} + allowed_bots: 'claude' + claude_args: '--allowedTools "Read,Grep,Glob,Bash,Write" --model ${{ env.ARBITER_MODEL }}' + prompt: ${{ steps.resolve.outputs.arbiter_prompt }} + env: + ANTHROPIC_BASE_URL: "https://openrouter.ai/api" + ANTHROPIC_AUTH_TOKEN: ${{ secrets.OPENROUTER_API_KEY }} + + - name: Record arbitration end time + if: always() + run: echo "ARBITER_END_NS=$(date +%s%N)" >> "$GITHUB_ENV" + + # Summarize how the panel split and whether arbitration went with it. This is the + # number worth watching: an arbitrator that never departs from the majority is a + # vote counter, and one that always does is ignoring the panel. + - name: Summarize panel outcome + if: always() + run: | + # Count with jq, not grep: the verdict is read from the parsed document, so a + # seat whose JSON is pretty-printed differently still counts. + approve=0; changes=0; seat_verdicts="" + for f in audit-*.json; do + [ -f "$f" ] || continue + seat="${f#audit-}"; seat="${seat%.json}" + v=$(jq -r '.verdict' "$f") + c=$(jq -r '.confidence // "unstated"' "$f") + case "$v" in + approve) approve=$((approve + 1)) ;; + request_changes) changes=$((changes + 1)) ;; + esac + seat_verdicts="${seat_verdicts:+$seat_verdicts,}$seat=$v/$c" + done + + echo "PANEL_APPROVE=$approve" >> "$GITHUB_ENV" + echo "PANEL_REQUEST_CHANGES=$changes" >> "$GITHUB_ENV" + # The seat-by-seat record, so a disagreement can be read back from telemetry + # without opening the run. + echo "PANEL_SEAT_VERDICTS=${seat_verdicts:-none}" >> "$GITHUB_ENV" + + if [ "$approve" -gt 0 ] && [ "$changes" -gt 0 ]; then + agreement="split" + elif [ "$changes" -gt 0 ]; then + agreement="unanimous_request_changes" + elif [ "$approve" -gt 0 ]; then + agreement="unanimous_approve" + else + agreement="none" + fi + echo "PANEL_AGREEMENT=$agreement" >> "$GITHUB_ENV" + echo "Panel: $approve approve, $changes request changes ($agreement) — ${seat_verdicts:-none}" - # 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. - name: Submit review as BiggiePockets id: submit env: # PAT authenticating as the BiggiePockets service account. A bot cannot # approve its own PR, so this never gates BiggiePockets-authored PRs. GH_TOKEN: ${{ secrets.BIGGIEPOCKETS_PAT }} - ASSIGNED_ARM: ${{ steps.resolve.outputs.assigned_arm }} run: | if [ ! -f verdict.json ]; then - echo "::error::Claude review run did not produce verdict.json" >&2 + echo "::error::Arbitration did not produce verdict.json" >&2 exit 1 fi @@ -372,31 +592,59 @@ jobs: gh pr review "$PR" --request-changes --body-file review-body.md ;; *) - echo "::error::Unexpected verdict from $ASSIGNED_ARM arm review: '$verdict'" >&2 + echo "::error::Unexpected verdict from arbitration: '$verdict'" >&2 exit 1 ;; esac echo "VERDICT=$verdict" >> "$GITHUB_ENV" - # Reports ONE LLM OBS TRACE per review, tagged with the arm this PR was assigned, - # and joined by a stable run_id (repo + pr + review attempt) so offline evals and - # panel ratings line up per review. Never blocks/fails/delays the review itself. - # - # `arm`/`prompt_name`/`prompt_version` are payload-level tags, which is where a tag - # key resolves to one value — one arm per review, so one value each. + # Whether arbitration followed the panel majority, computed here so the tag + # reflects the verdict actually posted. A tie has no majority to follow or + # depart from — which is exactly when the arbitrator is doing the deciding — so + # record that as its own value rather than scoring it as an override. + approve="${PANEL_APPROVE:-0}" + changes="${PANEL_REQUEST_CHANGES:-0}" + if [ "$changes" -gt "$approve" ]; then + majority="request_changes" + elif [ "$approve" -gt "$changes" ]; then + majority="approve" + else + majority="none" + fi + + if [ "$majority" = "none" ]; then + followed="no_majority" + elif [ "$verdict" = "$majority" ]; then + followed="true" + else + followed="false" + fi + echo "ARBITER_FOLLOWED_PANEL=$followed" >> "$GITHUB_ENV" + echo "Panel majority: $majority; arbitrated verdict: $verdict (followed=$followed)" + + # Reports ONE LLM OBS TRACE per review, joined by a stable run_id (repo + pr + + # review attempt) so offline evals and panel ratings line up per review. Never + # blocks/fails/delays the review itself. # - # root biggiepockets.review -> codex.review, claude.synthesize + # root biggiepockets.review + # ├── codex.review + # ├── magi.audit. (one span per auditor) + # └── magi.arbitrate # - # Compare arms by grouping on `arm` across pull requests (verdict rate, latency, - # tokens). + # The span tree is where a seat's individual verdict lives; the payload tags carry + # the panel-level outcome (agreement, split, whether arbitration followed the + # majority), which is what makes "is the panel earning its cost?" a query rather + # than a manual read of run logs. - name: Report review metrics to Datadog LLM Obs if: always() continue-on-error: true env: DD_API_KEY: ${{ secrets.DATADOG_API_KEY }} CODEX_PROMPT_TEMPLATE: ${{ steps.resolve.outputs.codex_prompt_template }} - ARM_PROMPT_TEMPLATE: ${{ steps.resolve.outputs.arm_prompt_template }} + AUDITOR_PROMPT_TEMPLATE: ${{ steps.resolve.outputs.auditor_prompt_template }} + ARBITER_PROMPT_TEMPLATE: ${{ steps.resolve.outputs.arbiter_prompt_template }} + AUDITORS_JSON: ${{ steps.resolve.outputs.auditors }} run: | if [ -z "$DD_API_KEY" ]; then echo "Skipping Datadog LLM Obs submission: DATADOG_API_KEY not configured" @@ -407,32 +655,24 @@ jobs: review_start_ns="${REVIEW_START_NS:-$now_ns}" codex_start_ns="${CODEX_START_NS:-$review_start_ns}" codex_end_ns="${CODEX_END_NS:-$codex_start_ns}" - claude_start_ns="${CLAUDE_START_NS:-$codex_end_ns}" - claude_end_ns="${CLAUDE_END_NS:-$claude_start_ns}" + arbiter_start_ns="${ARBITER_START_NS:-$codex_end_ns}" + arbiter_end_ns="${ARBITER_END_NS:-$arbiter_start_ns}" verdict="${VERDICT:-unknown}" codex_prompt_name="${{ steps.resolve.outputs.codex_prompt_name }}" codex_prompt_version="${{ steps.resolve.outputs.codex_prompt_version }}" - arm="${{ steps.resolve.outputs.assigned_arm }}" - arm_prompt_name="${{ steps.resolve.outputs.arm_prompt_name }}" - arm_prompt_version="${{ steps.resolve.outputs.arm_prompt_version }}" - control_arm="${{ steps.resolve.outputs.control_arm }}" - # Which side of the split this review is on, plus the inputs that produced it. - # Recording the bucket and the split percentage makes an assignment auditable - # after the fact: a later Apply that changes the split can't silently rewrite - # what an already-recorded review was assigned under. - assignment_bucket="${{ steps.resolve.outputs.assignment_bucket }}" - split_percent="${{ steps.resolve.outputs.experiment_split_percent }}" - arm_role="control" - [ "$arm" != "$control_arm" ] && arm_role="experiment" + auditor_prompt_name="${{ steps.resolve.outputs.auditor_prompt_name }}" + auditor_prompt_version="${{ steps.resolve.outputs.auditor_prompt_version }}" + arbiter_prompt_name="${{ steps.resolve.outputs.arbiter_prompt_name }}" + arbiter_prompt_version="${{ steps.resolve.outputs.arbiter_prompt_version }}" codex_status="ok" [ "${CODEX_STATUS:-failure}" != "success" ] && codex_status="error" codex_findings=$(cat codex-findings.md 2>/dev/null || true) codex_findings_lines=$(wc -l < codex-findings.md 2>/dev/null || echo 0) - claude_status="ok" - [ "${{ steps.claude.outcome }}" != "success" ] && claude_status="error" + arbiter_status="ok" + [ "${{ steps.arbiter.outcome }}" != "success" ] && arbiter_status="error" root_status="ok" [ "${{ steps.submit.outcome }}" != "success" ] && root_status="error" @@ -456,16 +696,18 @@ jobs: # Prompt Tracking: each LLM span carries the registry template that produced it, # under meta.input.prompt. The template keeps its {{PR}}/{{PROMPT_NAME}}/ - # {{PROMPT_VERSION}} placeholders and the substituted values ride along as - # `variables`, so every PR's run lands on the same prompt in Datadog instead of - # registering a new template per PR. `version` is our content-derived hash, so a - # prompt edit (a Roll) is what starts a new version. + # {{PROMPT_VERSION}}/{{AUDITORS}} placeholders and the substituted values ride + # along as `variables`, so every PR's run lands on the same prompt in Datadog + # instead of registering a new template per PR. `version` is our content-derived + # hash, so a prompt edit (a Roll) is what starts a new version. + auditor_list=$(printf '%s' "$AUDITORS_JSON" | jq -r 'join(", ")') prompt_json() { # name version template -> Prompt object jq -n \ --arg name "$1" \ --arg version "$2" \ --arg template "$3" \ --arg pr "$PR" \ + --arg auditors "$auditor_list" \ '{ id: $name, name: $name, @@ -474,47 +716,112 @@ jobs: # Report only the placeholders this template actually uses, so the # variables list matches what a Playground replay needs to render it. variables: ( - {PR: $pr, PROMPT_NAME: $name, PROMPT_VERSION: $version} + {PR: $pr, PROMPT_NAME: $name, PROMPT_VERSION: $version, AUDITORS: $auditors} | with_entries(select(.key as $k | $template | contains("{{" + $k + "}}"))) ) }' } codex_prompt_obj=$(prompt_json "$codex_prompt_name" "$codex_prompt_version" "$CODEX_PROMPT_TEMPLATE") - arm_prompt_obj=$(prompt_json "$arm_prompt_name" "$arm_prompt_version" "$ARM_PROMPT_TEMPLATE") + auditor_prompt_obj=$(prompt_json "$auditor_prompt_name" "$auditor_prompt_version" "$AUDITOR_PROMPT_TEMPLATE") + arbiter_prompt_obj=$(prompt_json "$arbiter_prompt_name" "$arbiter_prompt_version" "$ARBITER_PROMPT_TEMPLATE") - codex_model_tag="${CODEX_MODEL:-unspecified}" - claude_model_tag="${CLAUDE_MODEL:-unspecified}" + codex_model_tag="${CODEX_MODEL_USED:-unspecified}" + arbiter_model_tag="${ARBITER_MODEL:-unspecified}" 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) - - spans=$(jq -n \ + arbiter_span_id=$(openssl rand -hex 8) + + # One span per seat, built from that seat's own recorded verdict and timing, so + # a disagreement is inspectable per auditor in the trace instead of collapsed + # into the arbitrated result. + audit_spans="[]" + seat_tags="[]" + for f in audit-*.json; do + [ -f "$f" ] || continue + seat="${f#audit-}"; seat="${seat%.json}" + key=$(printf '%s' "$seat" | tr '[:lower:]-' '[:upper:]_') + eval "s_start=\${AUDIT_${key}_START_NS:-$codex_end_ns}" + eval "s_end=\${AUDIT_${key}_END_NS:-\$s_start}" + eval "s_status=\${AUDIT_${key}_STATUS:-error}" + eval "s_model=\${AUDIT_${key}_MODEL:-unspecified}" + [ "$s_status" = "ok" ] || s_status="error" + s_verdict=$(jq -r '.verdict // "unknown"' "$f") + s_confidence=$(jq -r '.confidence // "unstated"' "$f") + s_findings=$(jq -r '.blocking_findings // [] | length' "$f") + s_summary=$(truncate "$(jq -r '.summary // ""' "$f")") + + audit_spans=$(jq -n \ + --argjson spans "$audit_spans" \ + --arg trace_id "$trace_id" \ + --arg parent_id "$root_span_id" \ + --arg span_id "$(openssl rand -hex 8)" \ + --arg seat "$seat" \ + --arg model "$s_model" \ + --arg status "$s_status" \ + --arg summary "$s_summary" \ + --arg pr "$PR" \ + --argjson prompt "$auditor_prompt_obj" \ + --argjson start_ns "$s_start" \ + --argjson duration "$((s_end - s_start))" \ + '$spans + [{ + parent_id: $parent_id, + trace_id: $trace_id, + span_id: $span_id, + name: "magi.audit.\($seat)", + start_ns: $start_ns, + duration: $duration, + status: $status, + meta: { + kind: "llm", + model_name: $model, + model_provider: "openrouter", + input: { + messages: [{role: "user", content: "Audit pr:\($pr) independently as \($seat)"}], + prompt: $prompt + }, + output: {messages: [{role: "assistant", content: $summary}]} + } + }]') + + seat_tags=$(jq -n --argjson t "$seat_tags" \ + --arg seat "$seat" --arg v "$s_verdict" --arg c "$s_confidence" \ + --arg m "$s_model" --arg n "$s_findings" \ + '$t + [ + "auditor_verdict.\($seat):\($v)", + "auditor_confidence.\($seat):\($c)", + "auditor_model.\($seat):\($m)", + "auditor_blocking_findings.\($seat):\($n)" + ]') + done + + fixed_spans=$(jq -n \ --arg trace_id "$trace_id" \ --arg root_span_id "$root_span_id" \ --arg codex_span_id "$codex_span_id" \ - --arg claude_span_id "$claude_span_id" \ + --arg arbiter_span_id "$arbiter_span_id" \ --arg repo "$GITHUB_REPOSITORY" \ --arg pr "$PR" \ --arg base_ref "${BASE_REF:-unknown}" \ --arg codex_model "$codex_model_tag" \ - --arg claude_model "$claude_model_tag" \ + --arg arbiter_model "$arbiter_model_tag" \ --argjson review_start_ns "$review_start_ns" \ --argjson root_duration "$((now_ns - review_start_ns))" \ --argjson codex_start_ns "$codex_start_ns" \ --argjson codex_duration "$((codex_end_ns - codex_start_ns))" \ - --argjson claude_start_ns "$claude_start_ns" \ - --argjson claude_duration "$((claude_end_ns - claude_start_ns))" \ + --argjson arbiter_start_ns "$arbiter_start_ns" \ + --argjson arbiter_duration "$((arbiter_end_ns - arbiter_start_ns))" \ --arg codex_status "$codex_status" \ - --arg claude_status "$claude_status" \ + --arg arbiter_status "$arbiter_status" \ --arg root_status "$root_status" \ --arg codex_findings_excerpt "$codex_findings_excerpt" \ --arg summary_excerpt "$summary_excerpt" \ + --arg seat_verdicts "${PANEL_SEAT_VERDICTS:-none}" \ --argjson codex_prompt "$codex_prompt_obj" \ - --argjson arm_prompt "$arm_prompt_obj" \ + --argjson arbiter_prompt "$arbiter_prompt_obj" \ '[ { parent_id: "undefined", @@ -552,66 +859,75 @@ jobs: { parent_id: $root_span_id, trace_id: $trace_id, - span_id: $claude_span_id, - name: "claude.synthesize", - start_ns: $claude_start_ns, - duration: $claude_duration, - status: $claude_status, + span_id: $arbiter_span_id, + name: "magi.arbitrate", + start_ns: $arbiter_start_ns, + duration: $arbiter_duration, + status: $arbiter_status, meta: { kind: "llm", - model_name: $claude_model, + model_name: $arbiter_model, model_provider: "openrouter", input: { - messages: [{role: "user", content: $codex_findings_excerpt}], - prompt: $arm_prompt + messages: [{role: "user", content: "Panel verdicts: \($seat_verdicts)"}], + prompt: $arbiter_prompt }, output: {messages: [{role: "assistant", content: $summary_excerpt}]} } } ]') + spans=$(jq -n --argjson fixed "$fixed_spans" --argjson audits "$audit_spans" '$fixed + $audits') + payload=$(jq -n \ --arg ml_app "biggiepockets-review" \ --arg repo "$GITHUB_REPOSITORY" \ --arg pr "$PR" \ --arg run_id "$run_id" \ --arg verdict "$verdict" \ - --arg arm "$arm" \ - --arg arm_role "$arm_role" \ - --arg prompt_name "$arm_prompt_name" \ - --arg prompt_version "$arm_prompt_version" \ - --arg control_arm "$control_arm" \ - --arg assignment_bucket "$assignment_bucket" \ - --arg split_percent "$split_percent" \ + --arg agreement "${PANEL_AGREEMENT:-unknown}" \ + --arg followed "${ARBITER_FOLLOWED_PANEL:-unknown}" \ + --arg approve "${PANEL_APPROVE:-0}" \ + --arg changes "${PANEL_REQUEST_CHANGES:-0}" \ + --arg reporting "${PANEL_SEATS_REPORTING:-0}" \ + --arg declared "${PANEL_SEATS_DECLARED:-0}" \ + --arg auditor_prompt_name "$auditor_prompt_name" \ + --arg auditor_prompt_version "$auditor_prompt_version" \ + --arg arbiter_prompt_name "$arbiter_prompt_name" \ + --arg arbiter_prompt_version "$arbiter_prompt_version" \ + --arg arbiter_model "$arbiter_model_tag" \ --arg codex_model "$codex_model_tag" \ - --arg claude_model "$claude_model_tag" \ --arg codex_prompt_name "$codex_prompt_name" \ --arg codex_prompt_version "$codex_prompt_version" \ --argjson codex_findings_lines "$codex_findings_lines" \ + --argjson seat_tags "$seat_tags" \ --argjson spans "$spans" \ '{ data: { type: "span", attributes: { ml_app: $ml_app, - tags: [ + tags: ([ "repo:\($repo)", "pr:\($pr)", "run_id:\($run_id)", - "arm:\($arm)", - "arm_role:\($arm_role)", - "prompt_name:\($prompt_name)", - "prompt_version:\($prompt_version)", "verdict:\($verdict)", - "control_arm:\($control_arm)", - "assignment_bucket:\($assignment_bucket)", - "experiment_split_percent:\($split_percent)", + "panel_agreement:\($agreement)", + "arbiter_followed_panel:\($followed)", + "panel_approve:\($approve)", + "panel_request_changes:\($changes)", + "panel_seats_reporting:\($reporting)", + "panel_seats_declared:\($declared)", + "auditor_prompt_name:\($auditor_prompt_name)", + "auditor_prompt_version:\($auditor_prompt_version)", + "arbiter_prompt_name:\($arbiter_prompt_name)", + "arbiter_prompt_version:\($arbiter_prompt_version)", + "arbiter_model:\($arbiter_model)", "codex_model:\($codex_model)", - "claude_model:\($claude_model)", "codex_findings_lines:\($codex_findings_lines)", "codex_prompt_name:\($codex_prompt_name)", "codex_prompt_version:\($codex_prompt_version)" - ], + ] + $seat_tags), spans: $spans } } @@ -626,5 +942,5 @@ jobs: if [ "$http_code" -ge 300 ] 2>/dev/null || [ "$http_code" = "000" ]; then echo "::warning::Datadog LLM Obs submission failed (HTTP $http_code): $(cat /tmp/dd_response.json 2>/dev/null)" else - echo "Reported ${arm} arm to Datadog LLM Obs (trace ${trace_id}, prompt ${arm_prompt_name}@${arm_prompt_version}, run_id ${run_id})" + echo "Reported panel to Datadog LLM Obs (trace ${trace_id}, agreement ${PANEL_AGREEMENT:-unknown}, verdict ${verdict}, run_id ${run_id})" fi diff --git a/.github/workflows/validate-prompts.yml b/.github/workflows/validate-prompts.yml index 2131fa6..7f5dd2a 100644 --- a/.github/workflows/validate-prompts.yml +++ b/.github/workflows/validate-prompts.yml @@ -3,8 +3,8 @@ name: Validate prompt registry # Guard rails for the BiggiePockets review prompt registry (prompts/). Prompt # versions are derived from content, so this prevents the failure modes that a # hand-bumped version can't: a broken template, a dangling {{@include}}, a -# registry.json that references a prompt that doesn't exist, or a resolver that -# is not deterministic for a fixed PR number. +# registry.json that references a prompt that doesn't exist, a panel that can't be +# fanned out, or a resolver that is not deterministic for a fixed PR number. on: pull_request: @@ -17,37 +17,45 @@ on: jobs: validate: runs-on: ubuntu-latest + env: + REGISTRY_DIR: ${{ github.workspace }} steps: - name: Checkout uses: actions/checkout@v4 - name: Validate registry config run: | - jq -e '.version == 2' prompts/registry.json >/dev/null + jq -e '.version == 3' prompts/registry.json >/dev/null jq -e '.codex_prompt == "codex-first-pass"' prompts/registry.json >/dev/null - jq -e '.control_arm == "control"' prompts/registry.json >/dev/null - # An out-of-range split would route reviews to an arm that doesn't exist, or - # silently pin every review to one side of a test that looks live. - jq -e '.experiment_split_percent | type == "number" and . >= 0 and . <= 100' \ - prompts/registry.json >/dev/null - for arm in $(jq -r '.arms | keys[]' prompts/registry.json); do - prompt=$(jq -r --arg a "$arm" '.arms[$a]' prompts/registry.json) - test -f "prompts/$prompt.md" || { echo "prompts/$prompt.md missing for arm $arm"; exit 1; } + for field in codex_prompt auditor_prompt arbiter_prompt; do + name=$(jq -r --arg f "$field" '.[$f] // empty' prompts/registry.json) + test -n "$name" || { echo "registry.json missing $field"; exit 1; } + test -f "prompts/$name.md" || { echo "prompts/$name.md missing for $field"; exit 1; } done - echo "registry.json OK" + # A one-seat "panel" is a single reviewer wearing a panel's costume: the + # arbitrator would have nothing to weigh and the extra stage would be pure cost. + count=$(jq -r '.auditors | length' prompts/registry.json) + test "$count" -ge 2 || { echo "registry.json declares $count auditor(s); need >= 2"; exit 1; } + distinct=$(jq -r '.auditors | unique | length' prompts/registry.json) + test "$count" = "$distinct" || { echo "duplicate auditor seat names"; exit 1; } + # Seat names become artifact names, file names, and env-var suffixes downstream. + for seat in $(jq -r '.auditors[]' prompts/registry.json); do + printf '%s' "$seat" | grep -qE '^[a-z][a-z0-9-]*$' \ + || { echo "auditor seat '$seat' is not lowercase alphanumeric/dashes"; exit 1; } + done + echo "registry.json OK ($count auditors)" - name: Resolver resolves all prompts for e2e PR env: - REGISTRY_DIR: ${{ github.workspace }} PR: '12345' run: | : > /tmp/o1.txt GITHUB_OUTPUT=/tmp/o1.txt scripts/resolve-prompts.sh >/dev/null + test -s /tmp/o1.txt || { echo "resolver wrote no output" >&2; exit 1; } echo "resolve OK" - name: Resolver is deterministic for a fixed PR env: - REGISTRY_DIR: ${{ github.workspace }} PR: '12345' run: | : > /tmp/o2.txt @@ -61,7 +69,6 @@ jobs: - name: No unresolved template markers env: - REGISTRY_DIR: ${{ github.workspace }} PR: '12345' run: | : > /tmp/o4.txt @@ -72,10 +79,11 @@ jobs: # the runner overrides that one with its own per-step file. test -s /tmp/o4.txt || { echo "resolver wrote no output" >&2; exit 1; } # Only the *resolved* prompts must be marker-free. The *_prompt_template - # outputs keep their {{PR}}/{{PROMPT_NAME}}/{{PROMPT_VERSION}} placeholders on - # purpose — that is the template Datadog Prompt Tracking records. - for out in codex_prompt arm_prompt; do + # outputs keep their {{PR}}/{{PROMPT_NAME}}/{{PROMPT_VERSION}}/{{AUDITORS}} + # placeholders on purpose — that is the template Datadog Prompt Tracking records. + for out in codex_prompt auditor_prompt arbiter_prompt; do resolved=$(sed -n "/^$out<&2; exit 1; } if printf '%s' "$resolved" | grep -qE '\{\{'; then echo "Unresolved template markers remain in $out:" >&2 printf '%s' "$resolved" | grep -nE '\{\{' >&2 @@ -86,7 +94,6 @@ jobs: - name: Prompt templates are include-expanded and non-empty env: - REGISTRY_DIR: ${{ github.workspace }} PR: '12345' run: | : > /tmp/o5.txt @@ -96,7 +103,7 @@ jobs: # template. They keep {{PR}}-style placeholders, but a shared-rule include # must already be expanded — an unexpanded {{@...}} would ship a template # that no longer matches the text the model actually saw. - for out in codex_prompt_template arm_prompt_template; do + for out in codex_prompt_template auditor_prompt_template arbiter_prompt_template; do tmpl=$(sed -n "/^$out<&2; exit 1; } if printf '%s' "$tmpl" | grep -qE '\{\{@'; then @@ -107,76 +114,92 @@ jobs: done echo "templates include-expanded" - - name: Arms are distinct prompts + # The whole point of the panel is three INDEPENDENT judgments of the same question. + # If the resolver ever parameterized the auditor prompt by seat, the seats would be + # answering subtly different questions and a disagreement would no longer mean the + # auditors disagreed. Assert the emitted auditor prompt is one fixed text. + - name: Auditor prompt is identical for every seat + env: + PR: '12345' run: | - # Two arms pointing at the same prompt file would report an A/B result for a - # test that has no variable in it. - total=$(jq -r '.arms | keys | length' prompts/registry.json) - distinct=$(jq -r '.arms | [.[]] | unique | length' prompts/registry.json) - test "$total" = "$distinct" || { echo "two arms resolve to the same prompt"; exit 1; } - echo "arms distinct ($total)" + : > /tmp/o6.txt + GITHUB_OUTPUT=/tmp/o6.txt scripts/resolve-prompts.sh >/dev/null + # Exactly one auditor_prompt block is emitted, and it names no individual seat. + blocks=$(grep -c '^auditor_prompt<<' /tmp/o6.txt) + test "$blocks" = "1" || { echo "expected 1 auditor_prompt block, got $blocks" >&2; exit 1; } + prompt=$(sed -n '/^auditor_prompt<&2 + exit 1 + fi + done + echo "auditor prompt is seat-agnostic" - # Only the ASSIGNED arm's prompt is resolved per run, so a broken variant prompt - # would stay invisible until a PR happened to hash into it. Sweep PR numbers until - # every declared arm has been assigned at least once, and resolve each one. - - name: Every arm resolves for some PR + # The arbitrator has to know which seats to expect, or a seat that failed to + # produce a verdict reads as an absence rather than a gap. + - name: Arbitrator prompt names the panel env: - REGISTRY_DIR: ${{ github.workspace }} - GITHUB_REPOSITORY: BiggerPockets/validate + PR: '12345' run: | - declared=$(jq -r '.arms | keys | sort | join(" ")' prompts/registry.json) - want=$(jq -r '.arms | keys | length' prompts/registry.json) - split=$(jq -r '.experiment_split_percent' prompts/registry.json) - seen=""; count=0 - # Stop as soon as every arm has been covered — each resolve costs ~0.15s, and a - # sane split hits both arms within the first handful of PR numbers. - for pr in $(seq 1 500); do - [ "$count" -eq "$want" ] && break - out=/tmp/arm-$pr.txt - # The resolver APPENDS to GITHUB_OUTPUT, so start from an empty file or a - # second resolve doubles every key and the extracted value spans both. - : > "$out" - PR=$pr GITHUB_OUTPUT=$out scripts/resolve-prompts.sh >/dev/null - arm=$(sed -n '/^assigned_arm<&2; exit 1; } - printf '%s' "$resolved" | grep -qE '\{\{' && { echo "markers left in arm $arm" >&2; exit 1; } - echo "arm $arm resolves (first seen at pr $pr)" + : > /tmp/o7.txt + GITHUB_OUTPUT=/tmp/o7.txt scripts/resolve-prompts.sh >/dev/null + tmpl=$(sed -n '/^arbiter_prompt_template<&2; exit 1; } + resolved=$(sed -n '/^arbiter_prompt<&2; exit 1; } done - got=$(printf '%s\n' $seen | sort | tr '\n' ' ' | sed 's/ $//') - # A 0 or 100 split deliberately routes every review to one arm, so only demand - # full coverage when the split actually sends traffic both ways. - if [ "$split" -gt 0 ] && [ "$split" -lt 100 ]; then - test "$got" = "$declared" || { - echo "arms assigned over 500 PRs: '$got'; declared: '$declared'" >&2 - exit 1 - } - fi - echo "arm coverage OK ($got)" + echo "arbiter prompt names every seat" - - name: Assignment is stable for a fixed PR + # The audit job fans out over this value via fromJSON, so it must be a JSON array + # of strings — a malformed one fails the matrix at run time, after stage 1 has + # already been paid for. + - name: Auditors output is a matrix-ready JSON array env: - REGISTRY_DIR: ${{ github.workspace }} - GITHUB_REPOSITORY: BiggerPockets/validate PR: '12345' run: | - # A PR that changed arms between reviews would post two different review styles - # on the same PR and split its telemetry across both arms. - for i in 1 2 3; do - : > /tmp/assign-$i.txt - GITHUB_OUTPUT=/tmp/assign-$i.txt scripts/resolve-prompts.sh >/dev/null - sed -n '/^assigned_arm< /tmp/arm-only-$i.txt - done - diff /tmp/arm-only-1.txt /tmp/arm-only-2.txt - diff /tmp/arm-only-2.txt /tmp/arm-only-3.txt - echo "assignment stable ($(cat /tmp/arm-only-1.txt))" + : > /tmp/o8.txt + GITHUB_OUTPUT=/tmp/o8.txt scripts/resolve-prompts.sh >/dev/null + auditors=$(sed -n '/^auditors<= 2 and all(.[]; type == "string")' >/dev/null \ + || { echo "auditors output is not a JSON array of >=2 strings: $auditors" >&2; exit 1; } + count=$(sed -n '/^auditor_count<&2; exit 1; } + # Single line, so it survives being interpolated into the matrix expression. + test "$(printf '%s' "$auditors" | wc -l | tr -d ' ')" = "0" \ + || { echo "auditors output must be compact single-line JSON" >&2; exit 1; } + echo "auditors matrix-ready ($auditors)" + + - name: Resolver rejects a broken registry + run: | + # Each of these misconfigurations would otherwise surface as a confusing + # mid-review failure (or, for a one-seat panel, as a silently degraded review). + cp prompts/registry.json /tmp/registry.bak + restore() { cp /tmp/registry.bak prompts/registry.json; } + expect_fail() { + local label="$1" + if PR=1 GITHUB_OUTPUT=/tmp/reject.txt scripts/resolve-prompts.sh >/dev/null 2>&1; then + restore; echo "resolver accepted $label" >&2; exit 1 + fi + restore; echo "rejected: $label" + } + jq '.auditor_prompt = "does-not-exist"' /tmp/registry.bak > prompts/registry.json + expect_fail "auditor_prompt pointing at a missing file" + jq '.arbiter_prompt = null' /tmp/registry.bak > prompts/registry.json + expect_fail "a null arbiter_prompt" + jq '.auditors = ["caspar"]' /tmp/registry.bak > prompts/registry.json + expect_fail "a one-seat panel" + jq '.auditors = ["caspar", "caspar", "melchior"]' /tmp/registry.bak > prompts/registry.json + expect_fail "duplicate seat names" + jq '.auditors = ["Caspar", "melchior"]' /tmp/registry.bak > prompts/registry.json + expect_fail "a seat name that is not shell/artifact safe" - name: Roll bumps prompt versions (content-derived provenance) env: - REGISTRY_DIR: ${{ github.workspace }} PR: '1' run: | : > /tmp/before.txt @@ -187,10 +210,10 @@ jobs: for f in prompts/_shared/*.md; do git checkout -- "$f"; done test -s /tmp/before.txt && test -s /tmp/after.txt \ || { echo "resolver wrote no output" >&2; exit 1; } - before=$(sed -n '/^arm_prompt_version<&2 exit 1 fi - echo "shared-rule Roll bumps versions ($before -> $after)" \ No newline at end of file + echo "shared-rule Roll bumps versions ($before -> $after)" diff --git a/README.md b/README.md index 07c51b3..5beb37e 100644 --- a/README.md +++ b/README.md @@ -4,24 +4,77 @@ Org-wide GitHub defaults and shared reusable workflows. ## Reviews by BiggiePockets -`.github/workflows/biggiepockets-review.yml` is a **reusable** workflow that runs a -two-stage AI code review on a pull request: - -1. **Codex first pass** — reviews the diff against the PR's JIRA ticket and writes findings. -2. **Claude verify & synthesize** — validates Codex's findings, reviews the diff - independently (grepping for callers/tests, factoring in the existing PR discussion), - checks the change against the ticket's acceptance criteria, and decides a single verdict. - -The **BiggiePockets** service account then submits the resulting `approve` / -`request_changes` review on the PR. If the PR has no `BIG-XXXXX` key in its title (or the -ticket can't be fetched), the review degrades gracefully to a diff-based review instead of -failing. - -Codex and Claude run as separate GitHub Actions jobs. Codex uploads the reviewed commit's -diff, ticket/discussion context, and findings as a short-lived artifact; Claude downloads -that immutable handoff. If Claude is rate-limited, use **Re-run failed jobs** on the workflow -run. GitHub reruns only the Claude job, reusing the completed Codex pass instead of invoking -Codex again. +`.github/workflows/biggiepockets-review.yml` is a **reusable** workflow that reviews a pull +request with a **panel** of independent auditors and an arbitrator: + +1. **First pass** — Codex reviews the diff against the PR's JIRA ticket and writes leads to + `codex-findings.md`. +2. **Panel** — three auditors named **caspar**, **balthazar**, and **melchior** each audit + the change *independently*, running the *same* auditor prompt, and each writes its own + verdict (`approve` / `request_changes`, plus a stated confidence and its blocking + findings). Each verifies the first-pass leads itself, greps for callers and tests, and + factors in the existing PR discussion and the ticket's intent. +3. **Arbitration** — one arbitrator reads all three verdicts, verifies each blocking finding + against the code, weighs the panel, and issues the single verdict that gets posted. + +The **BiggiePockets** service account then submits that `approve` / `request_changes` review +on the PR. If the PR has no `BIG-XXXXX` key in its title (or the ticket can't be fetched), +the review degrades gracefully to a diff-based review instead of failing. + +**What makes the audits independent.** Each seat runs in its own GitHub Actions job, on its +own runner, and can read only the stage-1 handoff — never another seat's output. A seat +writes its verdict to `audit-.json` and uploads it; the arbitration job is the first +and only place in the run where more than one verdict exists together. So when two auditors +disagree, they disagree because they judged the code differently, not because one saw the +other's answer. + +The seats run **different models** on the identical prompt (see *Models* below). That is the +point: with the prompt held fixed, a split between seats is signal about the change, whereas +three runs of one model would mostly agree trivially and leave the arbitrator nothing to +weigh. + +**Arbitration weighs, it does not tally.** A blocking finding the arbitrator can confirm in +the code outweighs any number of approvals that missed it; one it can disprove is discarded +however confidently it was raised; and unanimity that the evidence doesn't support is +overruled with an explanation. The arbitrator may confirm or discard what the panel raised +but may not introduce a blocking issue no auditor found — it arbitrates rather than becoming +a fourth reviewer. The posted summary closes with a `Panel:` line recording how the seats +split and where the arbitrator overrode them. + +**Retries.** Stage 1 uploads the reviewed commit's diff, ticket/discussion context, and +first-pass findings as one short-lived artifact; every later job downloads that immutable +handoff, so all three auditors judge the exact same commit even if the PR is pushed to +mid-review. If a seat is rate-limited, use **Re-run failed jobs** on the workflow run: +GitHub re-runs only the failed seats, reusing the completed first pass. A seat that produces +no valid verdict is recorded as failed rather than counted as an approval, and arbitration +requires a **majority of seats** to have reported — below that it fails loudly instead of +quietly downgrading to a one-reviewer review. + +Requiring only a majority is what makes a tie possible: three binary verdicts can only land +3-0, 2-1, 1-2, or 0-3, but if one seat fails, arbitration runs over the two that reported and +those two can split 1-1. That is the case where the arbitrator decides alone, so it is tagged +`arbiter_followed_panel:no_majority` rather than scored as overriding a majority that never +existed. The alternative — demanding all three seats — would let one rate-limited seat fail +an entire review. + +### Models + +Every stage's model is an OpenRouter slug with a default pinned in the workflow, overrideable +per repo by setting the matching **Actions variable** (Settings → Actions → Variables) in the +calling repo. An unset or empty variable keeps the default. + +| Stage | Variable | Default | +| --- | --- | --- | +| First pass | `CODEX_MODEL` | `openai/gpt-5.6-sol` | +| caspar | `MAGI_MODEL_CASPAR` | `openai/gpt-5.6-luna` | +| balthazar | `MAGI_MODEL_BALTHAZAR` | `google/gemini-3.7-flash` | +| melchior | `MAGI_MODEL_MELCHIOR` | `z-ai/glm-5.3-flash` | +| Arbitration | `ARBITER_MODEL` | `openai/gpt-5.6-sol` | + +`MAGI_MODEL_DEFAULT` (default `google/gemini-3.7-flash`) covers a seat added to +`prompts/registry.json` that has no entry of its own, so a new seat runs instead of failing +the matrix. Changing a seat's model never touches the prompt — the auditor prompt is +seat-agnostic by construction, and CI asserts it. The review logic lives centrally in this repo. Each consuming repo only adds a thin **caller** workflow that owns the triggers and gating and delegates to this one. @@ -32,11 +85,12 @@ Do this once per repo you want BiggiePockets to review. #### 1. Install the Claude GitHub app -The Claude verification stage uses [`anthropics/claude-code-action`](https://github.com/anthropics/claude-code-action). +The auditor and arbitration stages use [`anthropics/claude-code-action`](https://github.com/anthropics/claude-code-action) +as their harness, pointed at OpenRouter model slugs. Install the official [Claude GitHub app](https://github.com/apps/claude) for the organization -once; you do **not** need to reinstall it per repo. Claude's model requests authenticate -through OpenRouter using the shared `OPENROUTER_API_KEY` described below, so no personal -Claude Code OAuth token is required. +once; you do **not** need to reinstall it per repo. Model requests authenticate through +OpenRouter using the shared `OPENROUTER_API_KEY` described below, so no personal Claude Code +OAuth token is required. #### 2. Add the caller workflow @@ -83,19 +137,19 @@ jobs: #### 3. Make the secrets available The reusable workflow consumes several secrets via `secrets: inherit`: credentials for the -the AI review provider (`OPENROUTER_API_KEY`, shared by both the Codex and Claude stages), -an Atlassian email + API token to fetch the PR's JIRA ticket for intent, and a personal access -token for the BiggiePockets service account that submits the review. Configure them as -**organization secrets** (recommended — set once, available to every repo) or as per-repo -secrets if you prefer to scope them. +AI review provider (`OPENROUTER_API_KEY`, shared by every stage — the first pass, all three +seats, and arbitration), an Atlassian email + API token to fetch the PR's JIRA ticket for +intent, and a personal access token for the BiggiePockets service account that submits the +review. Configure them as **organization secrets** (recommended — set once, available to +every repo) or as per-repo secrets if you prefer to scope them. It also reports per-review traces to the `biggiepockets-review` app in Datadog LLM -Observability via `secrets.DATADOG_API_KEY`: verdict, timing, prompt template and version -(tracked as prompts, see below), the model each -stage ran (`CODEX_MODEL`/`CLAUDE_MODEL` env vars in the workflow — both are OpenRouter -model slugs and must be set), and the actual findings text from Codex and the summary Claude wrote, -so review quality is inspectable, not just counted. This secret is optional — reviews still -run and post normally without it, but no metrics are reported. +Observability via `secrets.DATADOG_API_KEY`: the arbitrated verdict, how the panel split, +each seat's own verdict and confidence, timing per stage, prompt template and version +(tracked as prompts, see below), the model each stage ran, the first-pass findings text, and +the summary the arbitrator wrote — so review quality is inspectable, not just counted. This +secret is optional; reviews still run and post normally without it, but no metrics are +reported. The exact secret names each step expects are visible in the `env:` and `with:` blocks of [`.github/workflows/biggiepockets-review.yml`](.github/workflows/biggiepockets-review.yml). @@ -131,43 +185,39 @@ Once installed, trigger a review either way: workflow_dispatch** and pass the PR number. (Available once the caller file is on the repo's default branch.) -### Prompt registry and the prompt A/B test +### Prompt registry -The review-stage prompts are not inline in the workflow. They live in this repo under -`prompts/` and are resolved at runtime by `scripts/resolve-prompts.sh`: +The prompts are not inline in the workflow. They live in this repo under `prompts/` and are +resolved at runtime by `scripts/resolve-prompts.sh`: ``` prompts/ - registry.json # arms + control arm + split + codex prompt - codex-first-pass.md # Stage 1 prompt (template) - claude-synthesize.md # Stage 2 control arm (template) - claude-synthesize-thesis-first.md # Stage 2 thesis-first arm (template) + registry.json # which prompt runs at each stage + the panel seats + codex-first-pass.md # stage 1 prompt (template) + magi-audit.md # stage 2 prompt — every seat runs this one text (template) + magi-arbitrate.md # stage 3 prompt (template) _shared/{completeness,privacy,migration-data,perf,parsing,navigation}-rules.md # shared rule blocks ``` - **Templates + shared blocks.** Each prompt references the shared rule blocks via - `{{@prompts/_shared/.md}}`, so the Codex and Claude prompts can never drift out of - sync. Prompts resolve `{{PR}}`, `{{PROMPT_NAME}}`, `{{PROMPT_VERSION}}` too. + `{{@prompts/_shared/.md}}`, so the first-pass and auditor prompts can never drift + out of sync. Prompts also resolve `{{PR}}`, `{{PROMPT_NAME}}`, `{{PROMPT_VERSION}}`, and + `{{AUDITORS}}` (the seat list, which the arbitrator needs so a missing verdict reads as a + gap rather than an absence). +- **One auditor prompt, three seats.** The resolver emits `auditor_prompt` **once** and + every seat runs that same text — nothing in it is parameterized by seat. That is what + makes a disagreement between seats meaningful: the seats are answering the same question, + so the difference is the model and the judgment, not the wording. CI asserts the auditor + prompt names no seat. - **Content-derived versions.** `prompt_version` is a content hash of the template plus the shared blocks it includes — it changes only when that prompt's text changes, not per PR - or per arm, so Datadog LLM Obs can attribute quality to the exact prompt text that ran. -- **One arm per pull request.** Two Claude prompts sit in the registry — `control` and - `thesis-first` — and each review runs exactly one of them. `scripts/resolve-prompts.sh` - hashes `:` into a bucket 0-99 and assigns the PR to the experiment arm when - that bucket falls under `experiment_split_percent` (50 today), so the arms split traffic - evenly and every review costs a single Claude pass. Assignment is a pure function of - repo and PR number: re-running a review reuses the same arm, and one PR never sees two - review styles. -- **The assigned arm decides.** Whichever arm a PR draws writes the posted summary *and* - the approve / request-changes verdict. No separate control gate holds the decision back, - which is the tradeoff for one pass per review: an experiment prompt affects real review - outcomes on its share of PRs. Set `experiment_split_percent` to `0` to route every - review to `control_arm` without removing the arm. The arm is never named in the review - comment — a reviewer who knows which prompt wrote a summary can't judge it blind. -- **Comparison is between PRs, not within one.** No PR is reviewed twice, so there is no - paired A/B to diff on a single PR. Compare the arms by grouping Datadog on `arm` across - many reviews — verdict rate, latency, tokens. That needs volume before it means - anything; a gap in approve rate over a dozen PRs is noise. + and not per seat. One `auditor_prompt_version` therefore covers all three audit spans, and + Datadog LLM Obs can attribute quality to the exact prompt text that ran. +- **Panel seats.** `registry.json` lists the seats under `auditors`. The workflow fans its + audit job out over that list, so adding or removing a seat is a one-line registry change — + no workflow edit. The resolver rejects a panel of fewer than two seats, duplicate seat + names, and seat names that aren't safe as artifact names, file names, and env-var suffixes. + Seat models are set in the workflow, never here (see *Models* above). - **Prompt Tracking.** Every LLM span carries the prompt that produced it under `meta.input.prompt` — the registry template with its `{{PR}}`-style placeholders intact, plus the values that filled them as `variables`, plus `id`/`name`/`version`. Keeping the @@ -176,38 +226,49 @@ prompts/ volume, latency, tokens, and a version diff per prompt, and any span can be replayed in the Playground with its exact template and variables. A version starts when the prompt text changes (a Roll), since `version` is the same content hash reported as a tag. -- **Datadog.** Each review is one trace, tagged with the arm that ran it: +- **Datadog.** Each review is one trace, one span per stage and one per seat: ``` - biggiepockets.review → codex.review, claude.synthesize + biggiepockets.review + ├── codex.review + ├── magi.audit.caspar + ├── magi.audit.balthazar + ├── magi.audit.melchior + └── magi.arbitrate ``` - A tag key resolves to one value per submitted payload, so an `arm` tag is only - trustworthy while a payload carries a single arm — which it does by construction now - that one arm runs per review. Tags include `arm`, `arm_role` (`control`/`experiment`), - `prompt_name`, `prompt_version`, `verdict`, `assignment_bucket`, and - `experiment_split_percent`. Recording the bucket and the split keeps an assignment - auditable: changing the split later can't rewrite what an already-recorded review ran - under. A stable `run_id` (`repo-pr-runid`) joins offline evals and panel ratings to the - exact review. + A seat's own verdict, confidence, model, and blocking-finding count live on that seat's + span and on `auditor_verdict.` / `auditor_confidence.` / + `auditor_model.` / `auditor_blocking_findings.` tags, so a disagreement is + queryable without opening the run. Panel-level tags carry the outcome: `verdict` (what was + posted), `panel_agreement` (`unanimous_approve` / `unanimous_request_changes` / `split`), + `panel_approve`, `panel_request_changes`, `panel_seats_reporting` / + `panel_seats_declared`, and `arbiter_followed_panel` (`true` / `false` / `no_majority`). + A stable `run_id` (`repo-pr-runid`) joins offline evals and panel ratings to the exact + review. + + **The two numbers worth watching** are `panel_agreement` and `arbiter_followed_panel`. + If the panel is nearly always unanimous, the seats aren't adding independent information + and the extra seats aren't earning their cost — diversify the models. If the arbitrator + never departs from the majority it is a vote counter, and if it always does it is ignoring + the panel; either way the arbitration prompt is the thing to fix. - Spans carrying an `arm_agreement`, `experiment_verdict`, or `label_assignment` tag came - from an earlier setup and are not comparable to these; exclude them when grouping by - arm. + Spans carrying `arm`, `arm_role`, `arm_agreement`, `experiment_verdict`, or + `label_assignment` tags came from the earlier A/B setup and are not comparable to these; + exclude them when querying. -**Registry operations** (kept distinct so a formatting experiment can't silently change the -production prompt): +**Registry operations:** - **Roll** — edit a prompt or shared-rule file; its content-derived `prompt_version` bumps. -- **Apply** — point `control_arm` or an arm at a different stored prompt in - `registry.json`, or change `experiment_split_percent` (no version change). Callers tracking `@main` pick the change up on their next run. A - caller that pins `uses:` to a tag or SHA needs BOTH that `@ref` and its `registry_ref` - input bumped in lockstep — Apply owns that ref-bump explicitly. -- **Split** — add a new arm entry in `registry.json` + its prompt file, and give it a - share of traffic via `experiment_split_percent`. -- **Merge** — fold a variant's content into another prompt and remove the arm. +- **Apply** — point `codex_prompt`, `auditor_prompt`, or `arbiter_prompt` at a different + stored prompt (no version change). Callers tracking `@main` pick the change up on their + next run. A caller that pins `uses:` to a tag or SHA needs BOTH that `@ref` and its + `registry_ref` input bumped in lockstep — Apply owns that ref-bump explicitly. +- **Seat** — add or remove a name in `auditors`, and give a new seat a `MAGI_MODEL_` + default in the workflow (without one it falls back to `MAGI_MODEL_DEFAULT`). A `validate-prompts.yml` workflow guards the registry: it fails a PR if a template has -dangling includes, `registry.json` references a missing prompt, two arms point at the -same prompt, an arm's prompt fails to resolve, arm assignment isn't stable for a fixed PR, -the resolver isn't deterministic, or a shared-rule edit doesn't bump versions. +dangling includes, `registry.json` references a missing prompt, the auditor prompt names a +seat, the arbitrator prompt doesn't name every seat, the seat list isn't a matrix-ready JSON +array of at least two safe unique names, a broken registry resolves instead of erroring, the +resolver isn't deterministic for a fixed PR, or a shared-rule edit doesn't bump versions. diff --git a/prompts/claude-synthesize.md b/prompts/claude-synthesize.md deleted file mode 100644 index 14ea29e..0000000 --- a/prompts/claude-synthesize.md +++ /dev/null @@ -1,53 +0,0 @@ -Synthesize a single review decision for pull request #{{PR}}. - -1. Read ticket.json. If "available" is true, treat its summary and description as the - intended behavior, and its acceptance_criteria (Atlassian Document Format JSON) as - GUIDELINES rather than a hard contract. The AC describe the minimum the ticket set out to - achieve, not the ceiling on what counts as correct: don't reject a change merely for not - matching them verbatim. Judge whether the change satisfies the ticket's intent, and weigh - git history and the PR discussion (steps 3-4) as more authoritative evidence of what was - actually meant. -2. Read Codex's first-pass findings in codex-findings.md (if missing or empty, proceed - with your own review). -3. Read conversations.json: the PR's existing discussion (issue_comments, review_comments - with file/line, and prior reviews). Factor this context into your review: respect - decisions already settled in the thread, don't re-raise concerns the author has already - addressed or that were explicitly accepted, and weigh unresolved objections raised by - human reviewers. Treat the conversation as more reliable than the ticket's literal - acceptance criteria: if the thread shows the work was refined or explicitly accepted - beyond the AC in a sound way, honor that. -4. Independently review the diff in pr.diff. Do NOT rely on the hunks alone: use `git log` / - `git show` on the branch to inspect commit history and grep for callers and related tests - to judge impact. - When the diff adds or changes image assets, check that they are provided at retina - quality (e.g. a 2x asset or an `srcset`/`@2x` variant); flag raster images that ship - only at 1x where a high-DPI version is expected. {{@prompts/_shared/migration-data-rule.md}} -5. Enforce task completeness and reject incomplete tasks, half-measures, placeholders, or - deferred work per these rules as blocking issues: - {{@prompts/_shared/completeness-rules.md}} -6. Enforce these BiggerPockets member-privacy rules and treat a genuine violation as a - blocking issue: - {{@prompts/_shared/privacy-rules.md}} -7. Check for batch/task performance hot spots per these rules — an N+1 or O(n^2)+ - pattern on a full-table #process/#collection is a genuine finding, not a nitpick; - flag it with a suggested fix: - {{@prompts/_shared/perf-rules.md}} -8. Check how the diff parses structured values, per these rules: - {{@prompts/_shared/parsing-rules.md}} -9. Check that in-app navigational links use React Router's Link rather than a raw `` - tag, per these rules: - {{@prompts/_shared/navigation-rules.md}} -10. Validate which of Codex's findings are real (discard false positives), add any genuine - issues Codex missed, and (when a ticket is available) judge genuine misses of the ticket's - intent or clear scope creep — but give credit when the author went beyond the literal - acceptance criteria in a sound way rather than flagging it as non-compliant. -11. Decide ONE verdict. Be pragmatic: use "request_changes" only when there is at least one - genuine, blocking issue (such as a bug, regression, privacy violation, half-finished task, - placeholder, or deferred work); otherwise "approve". A change that exceeds the AC without - breaking the ticket's intent is a reason to approve, not to block. - -Write a file named verdict.json in the current working directory with EXACTLY this shape: - {"verdict": "approve" | "request_changes", "summary": ""} -In the summary, note which of Codex's findings you confirmed, anything you added, how the -change measures against the ticket, how the existing PR discussion informed your decision, -and the rationale for the decision. \ No newline at end of file diff --git a/prompts/magi-arbitrate.md b/prompts/magi-arbitrate.md new file mode 100644 index 0000000..ea8d98a --- /dev/null +++ b/prompts/magi-arbitrate.md @@ -0,0 +1,53 @@ +You are the arbitrator for the review of pull request #{{PR}}. Three auditors — +{{AUDITORS}} — audited this change independently, without seeing each other's work. Each +wrote a verdict. Your job is to weigh those verdicts against the evidence and issue the ONE +verdict that gets posted as the review. Yours is the only output that reaches the pull +request. + +1. Read every audit verdict: for each auditor in {{AUDITORS}} there is a file + `audit-.json` in the current working directory, with this shape: + {"verdict", "confidence", "blocking_findings": [{"file", "line", "issue"}], "summary"} + If one of those files is missing or unparseable, say so explicitly in your summary and + arbitrate over the verdicts you do have — never silently treat a missing auditor as an + approval. +2. Read the evidence the auditors worked from: pr.diff, ticket.json (its acceptance_criteria + are guidelines, not a hard contract), conversations.json, and codex-findings.md. +3. Verify each distinct blocking finding against the code yourself. Use `git log` / `git + show` on the branch and grep for callers and tests. Findings often overlap: treat two + auditors describing the same defect at the same location as ONE finding raised twice, not + two findings. +4. Weigh, do not tally. A verdict's weight comes from its evidence, not from how many + auditors share it: + - A single blocking finding you can confirm in the code outweighs any number of approvals + that simply did not notice it. + - A blocking finding you can disprove — the auditor misread the code, the concern was + already settled in the PR discussion, or the "missing" work is present elsewhere in the + diff — is discarded no matter how confidently it was raised. + - A stated confidence of "low" is a reason to verify that auditor's reasoning yourself + before it moves your verdict, in either direction. + - Unanimity is not proof. If all three agree and the evidence does not support them, follow + the evidence and explain why you departed. +5. Decide ONE verdict. Use "request_changes" only when at least one blocking issue survives + your verification; otherwise "approve". Be pragmatic: a change that exceeds the ticket's + acceptance criteria without breaking its intent is a reason to approve, not to block. +6. Arbitrate; do not re-review. You may confirm or discard what the auditors raised, but do + not introduce a blocking issue no auditor raised. If you spot one, record it in the summary + as a non-blocking observation for the author and leave the verdict to the findings the + panel actually surfaced. + +Write a file named verdict.json in the current working directory with EXACTLY this shape: + {"verdict": "approve" | "request_changes", "summary": ""} + +Structure the summary for the pull request author: + +- OPEN with a 1-3 sentence thesis stating the verdict and the single most important reason. +- Then one short paragraph per surviving blocking finding: the file/line, why it matters, and + the suggested direction. If nothing survived, say so. +- CLOSE with a short "Panel" line recording how the auditors split and where you overrode + them, e.g. "Panel: 2 approve, 1 request changes. Sustained melchior's finding on the + unbatched query; discarded caspar's nil-guard concern, which the diff already handles at + app/models/foo.rb:41." +- Cover how the change measures against the ticket and how the existing PR discussion + informed the outcome. Aim for roughly 200-250 words. + +Do not name which model sat in which seat, and do not write any other file. diff --git a/prompts/claude-synthesize-thesis-first.md b/prompts/magi-audit.md similarity index 58% rename from prompts/claude-synthesize-thesis-first.md rename to prompts/magi-audit.md index ed4557a..7735d31 100644 --- a/prompts/claude-synthesize-thesis-first.md +++ b/prompts/magi-audit.md @@ -1,4 +1,8 @@ -Synthesize a single review decision for pull request #{{PR}}. +You are one of three independent auditors reviewing pull request #{{PR}}. Audit the +change on its own merits and reach your own verdict. You have no visibility into the +other auditors and must not speculate about what they concluded — a separate arbitrator +weighs all three verdicts afterwards. Your job is an honest, self-contained judgment, +not a guess at consensus. 1. Read ticket.json. If "available" is true, treat its summary and description as the intended behavior, and its acceptance_criteria (Atlassian Document Format JSON) as @@ -7,8 +11,9 @@ Synthesize a single review decision for pull request #{{PR}}. matching them verbatim. Judge whether the change satisfies the ticket's intent, and weigh git history and the PR discussion (steps 3-4) as more authoritative evidence of what was actually meant. -2. Read Codex's first-pass findings in codex-findings.md (if missing or empty, proceed - with your own review). +2. Read the first-pass findings in codex-findings.md (if missing or empty, proceed with + your own review). These are leads to verify, not conclusions to adopt: confirm each one + against the code yourself and discard the false positives. 3. Read conversations.json: the PR's existing discussion (issue_comments, review_comments with file/line, and prior reviews). Factor this context into your review: respect decisions already settled in the thread, don't re-raise concerns the author has already @@ -37,28 +42,35 @@ Synthesize a single review decision for pull request #{{PR}}. 9. Check that in-app navigational links use React Router's Link rather than a raw `` tag, per these rules: {{@prompts/_shared/navigation-rules.md}} -10. Validate which of Codex's findings are real (discard false positives), add any genuine - issues Codex missed, and (when a ticket is available) judge genuine misses of the ticket's - intent or clear scope creep — but give credit when the author went beyond the literal - acceptance criteria in a sound way rather than flagging it as non-compliant. +10. Add any genuine issues the first pass missed, and (when a ticket is available) judge + genuine misses of the ticket's intent or clear scope creep — but give credit when the + author went beyond the literal acceptance criteria in a sound way rather than flagging it + as non-compliant. 11. Decide ONE verdict. Be pragmatic: use "request_changes" only when there is at least one genuine, blocking issue (such as a bug, regression, privacy violation, half-finished task, placeholder, or deferred work); otherwise "approve". A change that exceeds the AC without breaking the ticket's intent is a reason to approve, not to block. Write a file named verdict.json in the current working directory with EXACTLY this shape: - {"verdict": "approve" | "request_changes", "summary": ""} -Write the summary in a THESIS-FIRST structure: + { + "verdict": "approve" | "request_changes", + "confidence": "high" | "medium" | "low", + "blocking_findings": [ + {"file": "", "line": "", "issue": ""} + ], + "summary": "" + } -- OPEN with a 1-3 sentence thesis paragraph stating the verdict and the single most - important reason, e.g. "Requesting changes: the schema migration also updates data and - should be a one-off rake task — see details below." -- Follow with ONE short supporting paragraph per finding. Each paragraph explains, in a - sentence or two: the file/line, why it matters, and the suggested direction. If there is - nothing to change, say so in the thesis paragraph and briefly note you have no blocking - findings. -- Cover the same substance a normal review would (which Codex findings you confirmed, - anything you added, how the change measures against the ticket, how the existing PR - discussion informed your decision) but prioritized and compressed: the thesis first, then - details. Target roughly 150-200 words total, always leading with the verdict. \ No newline at end of file +- "blocking_findings" lists ONLY the issues you consider blocking, and must be empty when + your verdict is "approve". Non-blocking observations belong in the summary. +- "confidence" is how sure you are of your own verdict. Say "low" when the diff's impact + turned on code you could not inspect, and reserve "high" for a verdict you could defend + line by line. The arbitrator weighs this, so an inflated confidence corrupts the panel. +- In the summary, state your rationale, which first-pass findings you confirmed or rejected + and why, anything you added, how the change measures against the ticket, and how the + existing PR discussion informed your decision. + +Write the file even if you found nothing: an approve with an empty blocking_findings list is +a complete verdict. Do not write any other file, and do not post a review or comment — the +arbitrator, not you, speaks to the pull request. diff --git a/prompts/registry.json b/prompts/registry.json index 4424aef..5a84b08 100644 --- a/prompts/registry.json +++ b/prompts/registry.json @@ -1,36 +1,34 @@ { "//": "Prompt registry config for the BiggiePockets review workflow.", "//": "", - "//": "Each review runs ONE Claude synthesize pass. Which arm's prompt it runs is", - "//": "decided per pull request: scripts/resolve-prompts.sh hashes ':' into a", - "//": "bucket 0-99, and a PR below experiment_split_percent is assigned the experiment", - "//": "arm instead of control_arm. The assigned arm writes the summary that gets posted", - "//": "AND the approve / request-changes decision, so an experiment arm's prompt does", - "//": "affect real review outcomes on its share of PRs. Set experiment_split_percent to", - "//": "0 to route every review to control_arm without deleting the arm.", + "//": "A review is a panel. Three auditors — caspar, balthazar, and melchior — each", + "//": "audit the change independently, in separate GitHub Actions jobs, running the", + "//": "SAME auditor prompt and each producing its own verdict. No auditor can see", + "//": "another's verdict: the audit outputs are uploaded per seat and are only ever", + "//": "downloaded by the arbitrator job that runs after all three finish.", "//": "", - "//": "Assignment is a pure function of repo and PR number, so re-running a review", - "//": "reuses the same arm and a PR never sees two different review styles.", + "//": "The arbitrator then reads all three verdicts, weighs them, and issues the ONE", + "//": "verdict that gets posted as the approve / request-changes review. It is the only", + "//": "stage whose output reaches the pull request.", "//": "", - "//": "Comparison is BETWEEN pull requests: Datadog tags each review with its assigned", - "//": "arm, so verdict rates and latency group by arm across PRs. No PR is ever reviewed", - "//": "by two arms, which is what keeps Claude token spend at one pass per review.", + "//": "auditors lists the seats. Adding or removing a name changes the panel size; the", + "//": "workflow fans out over this list and the arbitrator is told which seats to expect,", + "//": "so a seat that failed to produce a verdict is a hard error rather than a silently", + "//": "smaller panel. Every seat runs auditor_prompt verbatim — seats differ only in which", + "//": "model they run, which the workflow decides (see MAGI_MODEL_* in", + "//": ".github/workflows/biggiepockets-review.yml), never this file.", "//": "", "//": "prompts/.md are versioned templates; shared rule blocks live in", "//": "prompts/_shared/ and are injected via {{@path}} markers. Prompt versions are", "//": "derived from file content by scripts/resolve-prompts.sh, never listed here.", "//": "", "//": "Operations: Roll = edit a prompt file (its derived version bumps). Apply = point", - "//": "control_arm or an arm at a different prompt file (no version change). Split = add", - "//": "an arm entry + prompt file, and give it a share via experiment_split_percent.", - "//": "Merge = fold a variant into another prompt file and remove the arm. Every", - "//": "consuming repo resolves this one file, so a change here changes them all at once.", - "version": 2, + "//": "codex_prompt, auditor_prompt, or arbiter_prompt at a different prompt file (no", + "//": "version change). Every consuming repo resolves this one file, so a change here", + "//": "changes them all at once.", + "version": 3, "codex_prompt": "codex-first-pass", - "control_arm": "control", - "experiment_split_percent": 50, - "arms": { - "control": "claude-synthesize", - "thesis-first": "claude-synthesize-thesis-first" - } + "auditor_prompt": "magi-audit", + "arbiter_prompt": "magi-arbitrate", + "auditors": ["caspar", "balthazar", "melchior"] } diff --git a/scripts/resolve-prompts.sh b/scripts/resolve-prompts.sh index b41ecfc..3eb1c75 100755 --- a/scripts/resolve-prompts.sh +++ b/scripts/resolve-prompts.sh @@ -1,38 +1,38 @@ #!/usr/bin/env bash # resolve-prompts.sh — resolve the BiggiePockets review prompts from the registry. # -# prompts/registry.json declares the codex prompt, one or more arms (each a stored -# prompt template), the control arm, and what percentage of pull requests an -# experiment arm gets. This script picks ONE arm per review — hash(':') -# mod 100, below experiment_split_percent means the experiment arm — and emits only -# that arm's prompt. The assigned arm both posts its summary and decides the review, -# so arms are compared BETWEEN pull requests and each review costs one Claude pass. +# prompts/registry.json declares the codex prompt, the auditor prompt, the arbitrator +# prompt, and the panel seats. A review runs the auditor prompt once per seat (caspar, +# balthazar, melchior) in independent jobs, then the arbitrator prompt once over the +# three verdicts those jobs produced. This script emits every prompt each review needs; +# the workflow decides which model runs in which seat. +# +# The auditor prompt is emitted ONCE and every seat runs that same text. Nothing in it +# is parameterized by seat, so the three audits differ only by model and by the +# independent judgment of the run — never by wording. # # Shared rule blocks (privacy, migration-data, perf) are stored once in # prompts/_shared/ and injected into every prompt that references them via {{@path}} -# markers, so the Codex and Claude prompts can never drift out of sync. +# markers, so the Codex and auditor prompts can never drift out of sync. # # The resolved text substitutes late-binding placeholders used for provenance only: -# {{PR}}, {{PROMPT_NAME}}, {{PROMPT_VERSION}} +# {{PR}}, {{PROMPT_NAME}}, {{PROMPT_VERSION}}, {{AUDITORS}} # # Alongside each resolved text, the unsubstituted template (shared blocks expanded, -# those three placeholders left intact) is emitted as _prompt_template. It is -# what Datadog LLM Obs Prompt Tracking records as the prompt template, with the -# substituted values reported as its variables, so runs of one prompt group together -# instead of splitting into a new template per PR. +# those placeholders left intact) is emitted as _prompt_template. It is what +# Datadog LLM Obs Prompt Tracking records as the prompt template, with the substituted +# values reported as its variables, so runs of one prompt group together instead of +# splitting into a new template per PR. # # prompt_version is DERIVED from content, never hand-set: a hash of the template file # plus every shared block it includes. It changes if and only if that prompt's text -# changes (a Roll), stays stable across PRs, and never encodes an arm — the arm is -# reported separately so Datadog LLM Obs can hold both (prompt_name/version = source -# of truth for the text; the arm = the test condition). +# changes (a Roll) and stays stable across PRs, so Datadog can attribute a change in +# review quality to the exact prompt text that produced it. # -# Inputs (env): REGISTRY_DIR, PR, GITHUB_REPOSITORY (falls back to "unknown", which -# only changes which arm a PR lands in, never whether assignment is deterministic). -# Outputs: codex_prompt{,_name,_version,_template}; arm_prompt{,_name,_version,_template} -# for the ASSIGNED arm; assigned_arm (its key); control_arm; experiment_split_percent; -# assignment_bucket (0-99, so an assignment can be recomputed and audited from the tag -# alone). +# Inputs (env): REGISTRY_DIR, PR. +# Outputs: codex_prompt{,_name,_version,_template}; auditor_prompt{,_name,_version,_template}; +# arbiter_prompt{,_name,_version,_template}; auditors (JSON array of seat names, which the +# workflow feeds straight into its fan-out matrix); auditor_count. set -euo pipefail @@ -47,41 +47,49 @@ fi MAP_VERSION=$(jq -r '.version // empty' "$REGISTRY_FILE") CODEX_NAME=$(jq -r '.codex_prompt // empty' "$REGISTRY_FILE") -CONTROL_ARM=$(jq -r '.control_arm // empty' "$REGISTRY_FILE") -SPLIT_PERCENT=$(jq -r '.experiment_split_percent // 0' "$REGISTRY_FILE") -ARMS=() -while IFS= read -r arm; do ARMS+=("$arm"); done < <(jq -r '.arms | keys[]' "$REGISTRY_FILE") +AUDITOR_NAME=$(jq -r '.auditor_prompt // empty' "$REGISTRY_FILE") +ARBITER_NAME=$(jq -r '.arbiter_prompt // empty' "$REGISTRY_FILE") -for name in "$CODEX_NAME" "$CONTROL_ARM"; do +# Validate the config so a bad edit is caught at review time instead of silently +# running a stale prompt or an undersized panel. +: "${MAP_VERSION:?registry.json missing version}" +for field in codex_prompt auditor_prompt arbiter_prompt; do + name=$(jq -r --arg f "$field" '.[$f] // empty' "$REGISTRY_FILE") if [ -z "$name" ]; then - echo "::error::registry.json is missing codex_prompt or control_arm" >&2 + echo "::error::registry.json is missing $field" >&2 + exit 1 + fi + if [ ! -f "$REGISTRY_DIR/prompts/$name.md" ]; then + echo "::error::$field is '$name' but prompts/$name.md does not exist" >&2 exit 1 fi done -if ! printf '%s' "$SPLIT_PERCENT" | grep -qE '^[0-9]+$' || [ "$SPLIT_PERCENT" -gt 100 ]; then - echo "::error::experiment_split_percent must be an integer 0-100, got '$SPLIT_PERCENT'" >&2 + +if ! jq -e '.auditors | type == "array"' "$REGISTRY_FILE" >/dev/null; then + echo "::error::registry.json .auditors must be an array of seat names" >&2 exit 1 fi - -# Validate the config so a bad edit is caught at review time instead of silently -# running a stale/duplicate prompt. -: "${MAP_VERSION:?registry.json missing version}" -for arm in "${ARMS[@]}"; do - prompt=$(jq -r --arg a "$arm" '.arms[$a] // empty' "$REGISTRY_FILE") - if [ -z "$prompt" ]; then - echo "::error::Arm '$arm' has no prompt mapping in prompts/registry.json" >&2 - exit 1 - fi - if [ ! -f "$REGISTRY_DIR/prompts/$prompt.md" ]; then - echo "::error::Arm '$arm' maps to '$prompt' but prompts/$prompt.md does not exist" >&2 +AUDITORS=() +while IFS= read -r seat; do AUDITORS+=("$seat"); done < <(jq -r '.auditors[]' "$REGISTRY_FILE") +if [ "${#AUDITORS[@]}" -lt 2 ]; then + echo "::error::registry.json declares ${#AUDITORS[@]} auditor(s); a panel needs at least 2" >&2 + exit 1 +fi +# Seat names become artifact names and shell identifiers downstream, and a duplicate +# would have two jobs overwrite one seat's verdict — shrinking the panel invisibly. +for seat in "${AUDITORS[@]}"; do + if ! printf '%s' "$seat" | grep -qE '^[a-z][a-z0-9-]*$'; then + echo "::error::auditor seat '$seat' must be lowercase alphanumeric/dashes" >&2 exit 1 fi done -if ! printf '%s\n' "${ARMS[@]}" | grep -qx -- "$CONTROL_ARM"; then - echo "::error::control_arm '$CONTROL_ARM' is not a declared arm" >&2 +if [ "$(printf '%s\n' "${AUDITORS[@]}" | sort -u | wc -l)" -ne "${#AUDITORS[@]}" ]; then + echo "::error::registry.json .auditors contains duplicate seat names" >&2 exit 1 fi +AUDITOR_LIST=$(printf '%s, ' "${AUDITORS[@]}"); AUDITOR_LIST="${AUDITOR_LIST%, }" + # Expand {{@path}} includes (path relative to REGISTRY_DIR), recording each included # file on _INCLUDED (de-duplicated). A marker may appear inline within a line: text # before/after the marker is preserved and the included content is always followed by @@ -90,7 +98,7 @@ fi # recursive resolver. _INCLUDED=() expand_template() { - local file="$1" out="" line rest pre rel post + local file="$1" out="" line rest pre rel while IFS= read -r line || [ -n "$line" ]; do rest="$line" while [[ "$rest" == *'{{@'* ]]; do @@ -102,9 +110,11 @@ expand_template() { exit 1 fi seen=0 - for f in "${_INCLUDED[@]:-}"; do - if [ "$f" = "$incfile" ]; then seen=1; break; fi - done + if [ "${#_INCLUDED[@]}" -gt 0 ]; then + for f in "${_INCLUDED[@]}"; do + if [ "$f" = "$incfile" ]; then seen=1; break; fi + done + fi if [ "$seen" -eq 0 ]; then _INCLUDED+=("$incfile"); fi out+="$pre" out+="$(cat "$incfile")" @@ -122,7 +132,12 @@ prompt_version() { # name -> content-derived version (template + shared includes expand_template "$REGISTRY_DIR/prompts/$name.md" > /dev/null { cat "$REGISTRY_DIR/prompts/$name.md" - for f in "${_INCLUDED[@]:-}"; do cat "$f"; done + # Test the length, not "${_INCLUDED[@]:-}": that form expands an empty array to one + # empty string, and a prompt with no {{@}} includes (the arbitrator has none) would + # `cat ""` and abort the resolve under `set -e`. + if [ "${#_INCLUDED[@]}" -gt 0 ]; then + for f in "${_INCLUDED[@]}"; do cat "$f"; done + fi } | sha256sum | cut -c1-12 } @@ -139,6 +154,7 @@ prompt_text() { # name version -> resolved text text="${text//\{\{PR\}\}/$PR}" text="${text//\{\{PROMPT_NAME\}\}/$name}" text="${text//\{\{PROMPT_VERSION\}\}/$version}" + text="${text//\{\{AUDITORS\}\}/$AUDITOR_LIST}" printf '%s' "$text" } @@ -158,30 +174,11 @@ write_output() { # name value — multiline-safe via GITHUB_OUTPUT heredoc; fixe } emit_prompt "codex" "$CODEX_NAME" +emit_prompt "auditor" "$AUDITOR_NAME" +emit_prompt "arbiter" "$ARBITER_NAME" -# Experiment arm = the first arm that is not the control arm; there may be none (a -# single-arm registry, e.g. after the experiment is merged away). -EXPERIMENT_ARM="" -for arm in "${ARMS[@]}"; do - if [ "$arm" != "$CONTROL_ARM" ]; then EXPERIMENT_ARM="$arm"; break; fi -done - -# Assign this PR to one arm. sha256 over ":" rather than the PR number alone, -# so two repos don't hand the same arm to their matching PR numbers, and the low 32 bits -# mod 100 give the bucket. Deterministic by construction: a re-review of the same PR -# resolves the same arm, and the bucket is emitted so any assignment can be re-derived -# from telemetry without rerunning this script. -ASSIGN_KEY="${GITHUB_REPOSITORY:-unknown}:$PR" -BUCKET=$(( 16#$(printf '%s' "$ASSIGN_KEY" | sha256sum | cut -c1-8) % 100 )) -ASSIGNED_ARM="$CONTROL_ARM" -if [ -n "$EXPERIMENT_ARM" ] && [ "$BUCKET" -lt "$SPLIT_PERCENT" ]; then - ASSIGNED_ARM="$EXPERIMENT_ARM" -fi - -write_output "control_arm" "$CONTROL_ARM" -write_output "experiment_split_percent" "$SPLIT_PERCENT" -write_output "assignment_bucket" "$BUCKET" -write_output "assigned_arm" "$ASSIGNED_ARM" -emit_prompt "arm" "$(jq -r --arg a "$ASSIGNED_ARM" '.arms[$a]' "$REGISTRY_FILE")" +# Emitted as compact JSON so the workflow can hand it directly to a matrix `include`. +write_output "auditors" "$(jq -c '.auditors' "$REGISTRY_FILE")" +write_output "auditor_count" "${#AUDITORS[@]}" -echo "Resolved $MAP_VERSION prompts: codex=$CODEX_NAME assigned=$ASSIGNED_ARM (bucket $BUCKET, split ${SPLIT_PERCENT}% to ${EXPERIMENT_ARM:-none})" +echo "Resolved v$MAP_VERSION prompts: codex=$CODEX_NAME auditor=$AUDITOR_NAME arbiter=$ARBITER_NAME panel=[$AUDITOR_LIST]" From aecfd9df8876b64e202ebe7f02cf5245b981ce58 Mon Sep 17 00:00:00 2001 From: Aaron Breckenridge Date: Fri, 28 Aug 2026 12:01:01 -0500 Subject: [PATCH 2/7] Post the arbitrated summary without a heading The review body opened with a '## BiggiePockets assisted review' heading. GitHub already attributes the review to BiggiePockets in its own chrome, so the heading restated the obvious and pushed the verdict itself below the fold. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/biggiepockets-review.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/biggiepockets-review.yml b/.github/workflows/biggiepockets-review.yml index a89810e..e6bccfd 100644 --- a/.github/workflows/biggiepockets-review.yml +++ b/.github/workflows/biggiepockets-review.yml @@ -582,7 +582,10 @@ jobs: verdict=$(jq -r '.verdict' verdict.json) summary=$(jq -r '.summary' verdict.json) - printf '## BiggiePockets assisted review\n\n%s\n' "$summary" > review-body.md + # The review body is the arbitrator's summary verbatim — no heading. GitHub + # already attributes the review to BiggiePockets in its own chrome, so a title + # restating that just pushes the verdict below the fold. + printf '%s\n' "$summary" > review-body.md case "$verdict" in approve) From e19c394c7c7351d6cc84e97311b1707d8b7c9d8b Mon Sep 17 00:00:00 2001 From: Aaron Breckenridge Date: Fri, 28 Aug 2026 12:08:35 -0500 Subject: [PATCH 3/7] Drop the pre-panel first pass; gather only ticket and PR context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The panel's value is three independent judgments. A Codex pass that read the diff first and handed the auditors a findings list undercut that: three seats mostly agreeing about one model's opinion is not independence, and the arbitrator was weighing verdicts that shared an origin. Stage 1 now runs no model at all — it gathers the diff, the JIRA ticket and its acceptance criteria, and the PR discussion, and the auditors are the first thing to read the code. Stage 1's job id is now `context` rather than `codex`, since it no longer runs Codex. A repo requiring `codex` as a status check must switch it to `context`. Removes prompts/codex-first-pass.md, `codex_prompt` from the registry (v4), the resolver's codex outputs, and the `codex.review` span and codex_* tags from the Datadog trace. Adds a CI check that no prompt reads a pre-existing findings file and that the registry declares no prompt but the auditor's and the arbitrator's, so a pre-pass cannot come back by accident. --- .github/workflows/biggiepockets-review.yml | 200 ++++++--------------- .github/workflows/validate-prompts.yml | 29 ++- README.md | 57 +++--- prompts/codex-first-pass.md | 38 ---- prompts/magi-arbitrate.md | 2 +- prompts/magi-audit.md | 36 ++-- prompts/registry.json | 14 +- scripts/resolve-prompts.sh | 25 ++- 8 files changed, 146 insertions(+), 255 deletions(-) delete mode 100644 prompts/codex-first-pass.md diff --git a/.github/workflows/biggiepockets-review.yml b/.github/workflows/biggiepockets-review.yml index e6bccfd..2d82125 100644 --- a/.github/workflows/biggiepockets-review.yml +++ b/.github/workflows/biggiepockets-review.yml @@ -4,8 +4,11 @@ name: BiggiePockets Code Review (reusable) # service account submitting the resulting approve / request-changes decision. # # A review runs in three stages: -# 1. First pass — Codex reads the diff against the ticket and writes leads to -# codex-findings.md. +# 1. Context — gathers what the panel judges against: the diff, the PR's JIRA ticket +# and its acceptance criteria, and the PR discussion. No model reviews +# the code here. Nothing reads the diff ahead of the panel, so no +# finding is planted in front of the auditors whose independence is the +# whole point of the panel. # 2. Panel — three auditors (caspar, balthazar, melchior) each audit the change # INDEPENDENTLY, in separate jobs, running the SAME auditor prompt, # and each writes its own verdict. No auditor can see another's @@ -22,11 +25,10 @@ name: BiggiePockets Code Review (reusable) # BIGGIEPOCKETS_PAT, DATADOG_API_KEY (optional; enables review-quality # metrics in Datadog LLM Obs). # -# Models: every stage's model is an OpenRouter slug pinned below and overrideable per -# repository without editing this workflow — set the matching Actions *variable* in the -# calling repo (Settings → Actions → Variables), which the `vars` context below reads. -# An unset or empty variable keeps the pinned default. -# CODEX_MODEL first pass (default openai/gpt-5.6-sol) +# Models: every model-running stage's model is an OpenRouter slug pinned below and +# overrideable per repository without editing this workflow — set the matching Actions +# *variable* in the calling repo (Settings → Actions → Variables), which the `vars` +# context below reads. An unset or empty variable keeps the pinned default. # MAGI_MODEL_CASPAR caspar (default openai/gpt-5.6-luna) # MAGI_MODEL_BALTHAZAR balthazar (default google/gemini-3.7-flash) # MAGI_MODEL_MELCHIOR melchior (default z-ai/glm-5.3-flash) @@ -36,8 +38,8 @@ name: BiggiePockets Code Review (reusable) # what a disagreement between seats measures is the models, and three runs of one model # would mostly agree trivially and leave the arbitrator nothing to weigh. # -# Prompt versioning: every prompt lives in this repo's prompts/ registry -# (BiggerPockets/.github) as a versioned template, plus one shared rule block per +# Prompt versioning: both prompts live in this repo's prompts/ registry +# (BiggerPockets/.github) as versioned templates, plus one shared rule block per # concern under prompts/_shared/ injected via {{@path}}. Prompts are resolved by # scripts/resolve-prompts.sh into step outputs; a derived prompt_version (content hash) # identifies each prompt in Datadog LLM Obs. Each LLM span also carries its template and @@ -64,15 +66,18 @@ on: type: string jobs: - # Stage 1. Gathers every input the panel reads and runs the Codex first pass, then - # uploads the lot as one immutable handoff. Separate from the panel jobs because GitHub - # can only re-run a failed JOB (not an individual step): this boundary lets a - # rate-limited auditor be retried without paying for another Codex pass or re-fetching - # the diff, and guarantees all three auditors judge the exact same commit and context. + # Stage 1. Gathers every input the panel reads — the diff, the JIRA ticket, the PR + # discussion — and uploads the lot as one immutable handoff. No model runs here: the + # panel is the first thing that forms an opinion about the code. # - # Job id stays `codex` so a repo that has this stage as a required status check keeps - # matching it. - codex: + # Separate from the panel jobs because GitHub can only re-run a failed JOB (not an + # individual step): this boundary lets a rate-limited auditor be retried without + # re-fetching the diff and ticket, and guarantees all three auditors judge the exact + # same commit and context. + # + # NOTE for callers: this job was previously named `codex`. A repo that lists it as a + # required status check must switch that check to `context`. + context: runs-on: ubuntu-latest outputs: head_sha: ${{ steps.pr_head.outputs.sha }} @@ -81,14 +86,8 @@ jobs: permissions: contents: read pull-requests: read - id-token: write # claude-code-action authenticates via OIDC env: PR: ${{ inputs.pr }} - # The first pass runs through OpenRouter, so its model must be an OpenRouter model - # slug and must be set explicitly — Codex's own default model name is not one - # OpenRouter accepts. - CODEX_MODEL: ${{ vars.CODEX_MODEL || 'openai/gpt-5.6-sol' }} - CODEX_RESPONSES_ENDPOINT: "https://openrouter.ai/api/v1/responses" steps: - name: Checkout PR head uses: actions/checkout@v4 @@ -96,8 +95,9 @@ jobs: ref: refs/pull/${{ inputs.pr }}/head fetch-depth: 0 - # Pin every later job to the exact commit the first pass read. This matters when a - # failed audit or arbitration job is retried after the PR has received another push. + # Pin every later job to the exact commit this context was gathered from. This + # matters when a failed audit or arbitration job is retried after the PR has + # received another push. - name: Record PR head SHA id: pr_head run: echo "sha=$(git rev-parse HEAD)" >> "$GITHUB_OUTPUT" @@ -200,81 +200,30 @@ jobs: > conversations.json echo "Wrote conversations.json ($(jq '.issue_comments | length' conversations.json) comments, $(jq '.review_comments | length' conversations.json) review comments, $(jq '.reviews | length' conversations.json) reviews)" - # Resolve the prompts from the registry — the first-pass prompt, the single auditor - # prompt every seat runs, and the arbitrator prompt — each with its shared-rule - # blocks expanded and a content-derived prompt_version. Also emits the panel seats, - # which drive the audit job's fan-out matrix. + # Resolve the prompts from the registry — the single auditor prompt every seat runs + # and the arbitrator prompt — each with its shared-rule blocks expanded and a + # content-derived prompt_version. Also emits the panel seats, which drive the audit + # job's fan-out matrix. - name: Resolve review prompts id: resolve env: REGISTRY_DIR: registry run: registry/scripts/resolve-prompts.sh - # Stage 1: Codex reviews the diff against the ticket intent and writes leads to - # codex-findings.md (the `output-file`, which captures Codex's final message). The - # panel treats these as leads to verify, not conclusions to adopt. - # - # Codex reviews the diff directly here rather than shelling out to the `codex review` - # subcommand. `codex review` starts its own in-process app-server that needs both a - # writable runtime home AND a localhost connection to the action's Responses API proxy. - # When it was launched as a child of `codex exec`, that child inherited the exec sandbox, - # which grants neither by default: under `sandbox: read-only` it died writing runtime - # state ("failed to initialize in-process app-server client: Read-only file system - # (os error 30)"), and under a writable profile it then died reaching the proxy - # ("stream disconnected before completion: error sending request for url - # http://127.0.0.1.../v1/responses"), because that profile grants filesystem writes but - # no network. Either way Codex produced no findings and every review silently degraded to - # a panel-only pass. Reviewing inline via this step's own `codex exec` model call avoids - # the nested subprocess entirely: it needs only to read files, so a read-only sandbox is - # sufficient and there is no child process to sandbox. - # - # continue-on-error keeps a Codex failure (e.g. an OpenAI "Quota exceeded" error) from - # blocking the review: the panel proceeds on its own when codex-findings.md is missing or - # empty. The next step still surfaces the failure as a warning so a genuinely broken Codex - # (e.g. a bad key) doesn't silently disappear. - - name: Record codex start time - run: echo "CODEX_START_NS=$(date +%s%N)" >> "$GITHUB_ENV" - - - name: Codex first-pass review - id: codex - continue-on-error: true - uses: openai/codex-action@v1 - with: - openai-api-key: ${{ secrets.OPENROUTER_API_KEY }} - responses-api-endpoint: ${{ env.CODEX_RESPONSES_ENDPOINT }} - model: ${{ env.CODEX_MODEL }} - sandbox: read-only - output-file: codex-findings.md - prompt: ${{ steps.resolve.outputs.codex_prompt }} - - # If Codex failed (it exits nonzero and writes no codex-findings.md — e.g. on a quota - # error), leave an empty findings file so the panel cleanly takes its no-findings path, - # and record a warning for observability. - - name: Note skipped Codex first pass - if: steps.codex.outcome == 'failure' + # Persist the handoff. Every auditor consumes the same diff and ticket/discussion + # context, on every retry. + # No `if: always()` on this or the upload: a stage that failed part-way through + # must not hand the panel a half-built context. Fail here and the whole review + # stops, which is the correct outcome. + - name: Record context metadata run: | - echo "::warning::Codex first-pass review failed (commonly an OpenAI quota error); continuing with a panel-only review." - : > codex-findings.md - - # Persist the handoff even when Codex failed. Every auditor consumes the same diff, - # ticket/discussion context, and (possibly empty) findings, on every retry. - - name: Record Codex completion metadata - if: always() - run: | - codex_end_ns=$(date +%s%N) - echo "CODEX_END_NS=$codex_end_ns" >> "$GITHUB_ENV" { echo "BASE_REF=$BASE_REF" echo "REVIEW_START_NS=$REVIEW_START_NS" - echo "CODEX_START_NS=$CODEX_START_NS" - echo "CODEX_END_NS=$codex_end_ns" - echo "CODEX_STATUS=${{ steps.codex.outcome }}" - echo "CODEX_MODEL_USED=$CODEX_MODEL" + echo "CONTEXT_END_NS=$(date +%s%N)" } > review-context.env - touch codex-findings.md - name: Upload review handoff - if: always() uses: actions/upload-artifact@v4 with: name: review-context-${{ github.run_id }} @@ -282,7 +231,6 @@ jobs: pr.diff ticket.json conversations.json - codex-findings.md review-context.env if-no-files-found: error retention-days: 7 @@ -297,12 +245,12 @@ jobs: # model step is continue-on-error so a rate-limited seat still leaves a recorded status # for the arbitrator. "Re-run failed jobs" re-runs only the seats that failed. audit: - needs: codex + needs: context runs-on: ubuntu-latest strategy: fail-fast: false matrix: - auditor: ${{ fromJSON(needs.codex.outputs.auditors) }} + auditor: ${{ fromJSON(needs.context.outputs.auditors) }} permissions: contents: read pull-requests: read @@ -321,14 +269,14 @@ jobs: - name: Checkout reviewed PR head uses: actions/checkout@v4 with: - ref: ${{ needs.codex.outputs.head_sha }} + ref: ${{ needs.context.outputs.head_sha }} fetch-depth: 0 - name: Checkout prompt registry uses: actions/checkout@v4 with: repository: BiggerPockets/.github - ref: ${{ needs.codex.outputs.registry_sha }} + ref: ${{ needs.context.outputs.registry_sha }} token: ${{ secrets.BIGGIEPOCKETS_PAT }} persist-credentials: false path: registry @@ -433,7 +381,7 @@ jobs: # Stage 3. Weighs the three verdicts and issues the one that gets posted. arbitrate: - needs: [codex, audit] + needs: [context, audit] runs-on: ubuntu-latest permissions: contents: read @@ -446,14 +394,14 @@ jobs: - name: Checkout reviewed PR head uses: actions/checkout@v4 with: - ref: ${{ needs.codex.outputs.head_sha }} + ref: ${{ needs.context.outputs.head_sha }} fetch-depth: 0 - name: Checkout prompt registry uses: actions/checkout@v4 with: repository: BiggerPockets/.github - ref: ${{ needs.codex.outputs.registry_sha }} + ref: ${{ needs.context.outputs.registry_sha }} token: ${{ secrets.BIGGIEPOCKETS_PAT }} persist-credentials: false path: registry @@ -631,7 +579,6 @@ jobs: # blocks/fails/delays the review itself. # # root biggiepockets.review - # ├── codex.review # ├── magi.audit. (one span per auditor) # └── magi.arbitrate # @@ -644,7 +591,6 @@ jobs: continue-on-error: true env: DD_API_KEY: ${{ secrets.DATADOG_API_KEY }} - CODEX_PROMPT_TEMPLATE: ${{ steps.resolve.outputs.codex_prompt_template }} AUDITOR_PROMPT_TEMPLATE: ${{ steps.resolve.outputs.auditor_prompt_template }} ARBITER_PROMPT_TEMPLATE: ${{ steps.resolve.outputs.arbiter_prompt_template }} AUDITORS_JSON: ${{ steps.resolve.outputs.auditors }} @@ -656,24 +602,16 @@ jobs: now_ns=$(date +%s%N) review_start_ns="${REVIEW_START_NS:-$now_ns}" - codex_start_ns="${CODEX_START_NS:-$review_start_ns}" - codex_end_ns="${CODEX_END_NS:-$codex_start_ns}" - arbiter_start_ns="${ARBITER_START_NS:-$codex_end_ns}" + context_end_ns="${CONTEXT_END_NS:-$review_start_ns}" + arbiter_start_ns="${ARBITER_START_NS:-$context_end_ns}" arbiter_end_ns="${ARBITER_END_NS:-$arbiter_start_ns}" verdict="${VERDICT:-unknown}" - codex_prompt_name="${{ steps.resolve.outputs.codex_prompt_name }}" - codex_prompt_version="${{ steps.resolve.outputs.codex_prompt_version }}" auditor_prompt_name="${{ steps.resolve.outputs.auditor_prompt_name }}" auditor_prompt_version="${{ steps.resolve.outputs.auditor_prompt_version }}" arbiter_prompt_name="${{ steps.resolve.outputs.arbiter_prompt_name }}" arbiter_prompt_version="${{ steps.resolve.outputs.arbiter_prompt_version }}" - codex_status="ok" - [ "${CODEX_STATUS:-failure}" != "success" ] && codex_status="error" - codex_findings=$(cat codex-findings.md 2>/dev/null || true) - codex_findings_lines=$(wc -l < codex-findings.md 2>/dev/null || echo 0) - arbiter_status="ok" [ "${{ steps.arbiter.outcome }}" != "success" ] && arbiter_status="error" @@ -683,9 +621,9 @@ jobs: summary="" [ -f verdict.json ] && summary=$(jq -r '.summary // ""' verdict.json 2>/dev/null) || summary="" - # Cap free-text fields sent to Datadog so one outsized findings report can't - # blow past the LLM Obs payload limit; the untruncated text is always still in - # this step's own job logs. + # Cap free-text fields sent to Datadog so one outsized summary can't blow past + # the LLM Obs payload limit; the untruncated text is always still in this step's + # own job logs. truncate() { local text="$1" max=4000 if [ "${#text}" -gt "$max" ]; then @@ -694,7 +632,6 @@ jobs: printf '%s' "$text" fi } - codex_findings_excerpt=$(truncate "$codex_findings") summary_excerpt=$(truncate "$summary") # Prompt Tracking: each LLM span carries the registry template that produced it, @@ -724,18 +661,15 @@ jobs: ) }' } - codex_prompt_obj=$(prompt_json "$codex_prompt_name" "$codex_prompt_version" "$CODEX_PROMPT_TEMPLATE") auditor_prompt_obj=$(prompt_json "$auditor_prompt_name" "$auditor_prompt_version" "$AUDITOR_PROMPT_TEMPLATE") arbiter_prompt_obj=$(prompt_json "$arbiter_prompt_name" "$arbiter_prompt_version" "$ARBITER_PROMPT_TEMPLATE") - codex_model_tag="${CODEX_MODEL_USED:-unspecified}" arbiter_model_tag="${ARBITER_MODEL:-unspecified}" 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) arbiter_span_id=$(openssl rand -hex 8) # One span per seat, built from that seat's own recorded verdict and timing, so @@ -747,7 +681,7 @@ jobs: [ -f "$f" ] || continue seat="${f#audit-}"; seat="${seat%.json}" key=$(printf '%s' "$seat" | tr '[:lower:]-' '[:upper:]_') - eval "s_start=\${AUDIT_${key}_START_NS:-$codex_end_ns}" + eval "s_start=\${AUDIT_${key}_START_NS:-$context_end_ns}" eval "s_end=\${AUDIT_${key}_END_NS:-\$s_start}" eval "s_status=\${AUDIT_${key}_STATUS:-error}" eval "s_model=\${AUDIT_${key}_MODEL:-unspecified}" @@ -804,26 +738,19 @@ jobs: fixed_spans=$(jq -n \ --arg trace_id "$trace_id" \ --arg root_span_id "$root_span_id" \ - --arg codex_span_id "$codex_span_id" \ --arg arbiter_span_id "$arbiter_span_id" \ --arg repo "$GITHUB_REPOSITORY" \ --arg pr "$PR" \ --arg base_ref "${BASE_REF:-unknown}" \ - --arg codex_model "$codex_model_tag" \ --arg arbiter_model "$arbiter_model_tag" \ --argjson review_start_ns "$review_start_ns" \ --argjson root_duration "$((now_ns - review_start_ns))" \ - --argjson codex_start_ns "$codex_start_ns" \ - --argjson codex_duration "$((codex_end_ns - codex_start_ns))" \ --argjson arbiter_start_ns "$arbiter_start_ns" \ --argjson arbiter_duration "$((arbiter_end_ns - arbiter_start_ns))" \ - --arg codex_status "$codex_status" \ --arg arbiter_status "$arbiter_status" \ --arg root_status "$root_status" \ - --arg codex_findings_excerpt "$codex_findings_excerpt" \ --arg summary_excerpt "$summary_excerpt" \ --arg seat_verdicts "${PANEL_SEAT_VERDICTS:-none}" \ - --argjson codex_prompt "$codex_prompt_obj" \ --argjson arbiter_prompt "$arbiter_prompt_obj" \ '[ { @@ -840,25 +767,6 @@ jobs: output: {value: $summary_excerpt} } }, - { - parent_id: $root_span_id, - trace_id: $trace_id, - span_id: $codex_span_id, - name: "codex.review", - start_ns: $codex_start_ns, - duration: $codex_duration, - status: $codex_status, - meta: { - kind: "llm", - model_name: $codex_model, - model_provider: "openrouter", - input: { - messages: [{role: "user", content: "Review pr:\($pr) against ticket.json/pr.diff"}], - prompt: $codex_prompt - }, - output: {messages: [{role: "assistant", content: $codex_findings_excerpt}]} - } - }, { parent_id: $root_span_id, trace_id: $trace_id, @@ -899,10 +807,6 @@ jobs: --arg arbiter_prompt_name "$arbiter_prompt_name" \ --arg arbiter_prompt_version "$arbiter_prompt_version" \ --arg arbiter_model "$arbiter_model_tag" \ - --arg codex_model "$codex_model_tag" \ - --arg codex_prompt_name "$codex_prompt_name" \ - --arg codex_prompt_version "$codex_prompt_version" \ - --argjson codex_findings_lines "$codex_findings_lines" \ --argjson seat_tags "$seat_tags" \ --argjson spans "$spans" \ '{ @@ -925,11 +829,7 @@ jobs: "auditor_prompt_version:\($auditor_prompt_version)", "arbiter_prompt_name:\($arbiter_prompt_name)", "arbiter_prompt_version:\($arbiter_prompt_version)", - "arbiter_model:\($arbiter_model)", - "codex_model:\($codex_model)", - "codex_findings_lines:\($codex_findings_lines)", - "codex_prompt_name:\($codex_prompt_name)", - "codex_prompt_version:\($codex_prompt_version)" + "arbiter_model:\($arbiter_model)" ] + $seat_tags), spans: $spans } diff --git a/.github/workflows/validate-prompts.yml b/.github/workflows/validate-prompts.yml index 7f5dd2a..9ca3abe 100644 --- a/.github/workflows/validate-prompts.yml +++ b/.github/workflows/validate-prompts.yml @@ -25,9 +25,8 @@ jobs: - name: Validate registry config run: | - jq -e '.version == 3' prompts/registry.json >/dev/null - jq -e '.codex_prompt == "codex-first-pass"' prompts/registry.json >/dev/null - for field in codex_prompt auditor_prompt arbiter_prompt; do + jq -e '.version == 4' prompts/registry.json >/dev/null + for field in auditor_prompt arbiter_prompt; do name=$(jq -r --arg f "$field" '.[$f] // empty' prompts/registry.json) test -n "$name" || { echo "registry.json missing $field"; exit 1; } test -f "prompts/$name.md" || { echo "prompts/$name.md missing for $field"; exit 1; } @@ -81,7 +80,7 @@ jobs: # Only the *resolved* prompts must be marker-free. The *_prompt_template # outputs keep their {{PR}}/{{PROMPT_NAME}}/{{PROMPT_VERSION}}/{{AUDITORS}} # placeholders on purpose — that is the template Datadog Prompt Tracking records. - for out in codex_prompt auditor_prompt arbiter_prompt; do + for out in auditor_prompt arbiter_prompt; do resolved=$(sed -n "/^$out<&2; exit 1; } if printf '%s' "$resolved" | grep -qE '\{\{'; then @@ -103,7 +102,7 @@ jobs: # template. They keep {{PR}}-style placeholders, but a shared-rule include # must already be expanded — an unexpanded {{@...}} would ship a template # that no longer matches the text the model actually saw. - for out in codex_prompt_template auditor_prompt_template arbiter_prompt_template; do + for out in auditor_prompt_template arbiter_prompt_template; do tmpl=$(sed -n "/^$out<&2; exit 1; } if printf '%s' "$tmpl" | grep -qE '\{\{@'; then @@ -114,6 +113,26 @@ jobs: done echo "templates include-expanded" + # Independence is only real if the auditors are the FIRST thing to read the diff. + # A pre-pass that hands the panel a list of findings makes three auditors agree + # about one model's opinion, which is the failure the panel exists to avoid. Assert + # the registry declares no prompt other than the auditor's and the arbitrator's, and + # that no prompt tells a model to read a pre-existing findings file. + - name: Nothing reviews the diff ahead of the panel + run: | + extra=$(jq -r 'to_entries + | map(select(.key | test("_prompt$"))) + | map(.key) + | map(select(. != "auditor_prompt" and . != "arbiter_prompt")) + | join(", ")' prompts/registry.json) + test -z "$extra" \ + || { echo "registry.json declares a pre-panel prompt: $extra" >&2; exit 1; } + if grep -rniE 'codex-findings|first[- ]pass|pre-?pass' prompts/; then + echo "a prompt references a pre-panel review pass" >&2 + exit 1 + fi + echo "no pre-panel review stage" + # The whole point of the panel is three INDEPENDENT judgments of the same question. # If the resolver ever parameterized the auditor prompt by seat, the seats would be # answering subtly different questions and a disagreement would no longer mean the diff --git a/README.md b/README.md index 5beb37e..8bd7408 100644 --- a/README.md +++ b/README.md @@ -7,16 +7,24 @@ Org-wide GitHub defaults and shared reusable workflows. `.github/workflows/biggiepockets-review.yml` is a **reusable** workflow that reviews a pull request with a **panel** of independent auditors and an arbitrator: -1. **First pass** — Codex reviews the diff against the PR's JIRA ticket and writes leads to - `codex-findings.md`. +1. **Context** — gathers what the panel judges against: the diff, the PR's JIRA ticket and + its acceptance criteria, and the PR's existing discussion. **No model reviews the code + here.** 2. **Panel** — three auditors named **caspar**, **balthazar**, and **melchior** each audit the change *independently*, running the *same* auditor prompt, and each writes its own verdict (`approve` / `request_changes`, plus a stated confidence and its blocking - findings). Each verifies the first-pass leads itself, greps for callers and tests, and - factors in the existing PR discussion and the ticket's intent. + findings). Each reads the diff cold, greps for callers and tests, and factors in the + existing PR discussion and the ticket's intent. 3. **Arbitration** — one arbitrator reads all three verdicts, verifies each blocking finding against the code, weighs the panel, and issues the single verdict that gets posted. +**Nothing reviews the diff ahead of the panel.** The auditors are the first thing to form an +opinion about the code, and stage 1 is deliberately model-free. A pre-pass that handed the +panel a list of findings would get three auditors agreeing about one model's opinion, which +is exactly the failure a panel exists to avoid — so CI asserts no prompt reads a +pre-existing findings file and the registry declares no prompt but the auditor's and the +arbitrator's. + The **BiggiePockets** service account then submits that `approve` / `request_changes` review on the PR. If the PR has no `BIG-XXXXX` key in its title (or the ticket can't be fetched), the review degrades gracefully to a diff-based review instead of failing. @@ -41,11 +49,11 @@ but may not introduce a blocking issue no auditor found — it arbitrates rather a fourth reviewer. The posted summary closes with a `Panel:` line recording how the seats split and where the arbitrator overrode them. -**Retries.** Stage 1 uploads the reviewed commit's diff, ticket/discussion context, and -first-pass findings as one short-lived artifact; every later job downloads that immutable -handoff, so all three auditors judge the exact same commit even if the PR is pushed to -mid-review. If a seat is rate-limited, use **Re-run failed jobs** on the workflow run: -GitHub re-runs only the failed seats, reusing the completed first pass. A seat that produces +**Retries.** Stage 1 uploads the reviewed commit's diff and ticket/discussion context as one +short-lived artifact; every later job downloads that immutable handoff, so all three auditors +judge the exact same commit even if the PR is pushed to mid-review. If a seat is rate-limited, +use **Re-run failed jobs** on the workflow run: GitHub re-runs only the failed seats, reusing +the context stage that already succeeded. A seat that produces no valid verdict is recorded as failed rather than counted as an approval, and arbitration requires a **majority of seats** to have reported — below that it fails loudly instead of quietly downgrading to a one-reviewer review. @@ -59,13 +67,12 @@ an entire review. ### Models -Every stage's model is an OpenRouter slug with a default pinned in the workflow, overrideable -per repo by setting the matching **Actions variable** (Settings → Actions → Variables) in the -calling repo. An unset or empty variable keeps the default. +Every model an OpenRouter slug with a default pinned in the workflow, overrideable per repo +by setting the matching **Actions variable** (Settings → Actions → Variables) in the calling +repo. An unset or empty variable keeps the default. Stage 1 runs no model, so it has no entry. | Stage | Variable | Default | | --- | --- | --- | -| First pass | `CODEX_MODEL` | `openai/gpt-5.6-sol` | | caspar | `MAGI_MODEL_CASPAR` | `openai/gpt-5.6-luna` | | balthazar | `MAGI_MODEL_BALTHAZAR` | `google/gemini-3.7-flash` | | melchior | `MAGI_MODEL_MELCHIOR` | `z-ai/glm-5.3-flash` | @@ -137,8 +144,8 @@ jobs: #### 3. Make the secrets available The reusable workflow consumes several secrets via `secrets: inherit`: credentials for the -AI review provider (`OPENROUTER_API_KEY`, shared by every stage — the first pass, all three -seats, and arbitration), an Atlassian email + API token to fetch the PR's JIRA ticket for +AI review provider (`OPENROUTER_API_KEY`, shared by all three seats and arbitration), an +Atlassian email + API token to fetch the PR's JIRA ticket for intent, and a personal access token for the BiggiePockets service account that submits the review. Configure them as **organization secrets** (recommended — set once, available to every repo) or as per-repo secrets if you prefer to scope them. @@ -146,8 +153,8 @@ every repo) or as per-repo secrets if you prefer to scope them. It also reports per-review traces to the `biggiepockets-review` app in Datadog LLM Observability via `secrets.DATADOG_API_KEY`: the arbitrated verdict, how the panel split, each seat's own verdict and confidence, timing per stage, prompt template and version -(tracked as prompts, see below), the model each stage ran, the first-pass findings text, and -the summary the arbitrator wrote — so review quality is inspectable, not just counted. This +(tracked as prompts, see below), the model each stage ran, and the summary the arbitrator +wrote — so review quality is inspectable, not just counted. This secret is optional; reviews still run and post normally without it, but no metrics are reported. @@ -176,6 +183,11 @@ write access. ### Using it +If your repo lists a review job as a **required status check**, the job names are `context`, +`audit ()` per seat, and `arbitrate`. Stage 1 was previously named `codex`; a branch +protection rule still requiring `codex` will block merges forever, since no job by that name +runs any more. + Once installed, trigger a review either way: - **Request a review** — add **BiggiePockets** as a reviewer on the PR. The workflow fires @@ -193,15 +205,14 @@ resolved at runtime by `scripts/resolve-prompts.sh`: ``` prompts/ registry.json # which prompt runs at each stage + the panel seats - codex-first-pass.md # stage 1 prompt (template) magi-audit.md # stage 2 prompt — every seat runs this one text (template) magi-arbitrate.md # stage 3 prompt (template) _shared/{completeness,privacy,migration-data,perf,parsing,navigation}-rules.md # shared rule blocks ``` - **Templates + shared blocks.** Each prompt references the shared rule blocks via - `{{@prompts/_shared/.md}}`, so the first-pass and auditor prompts can never drift - out of sync. Prompts also resolve `{{PR}}`, `{{PROMPT_NAME}}`, `{{PROMPT_VERSION}}`, and + `{{@prompts/_shared/.md}}`, so one edit updates every prompt that enforces that + rule. Prompts also resolve `{{PR}}`, `{{PROMPT_NAME}}`, `{{PROMPT_VERSION}}`, and `{{AUDITORS}}` (the seat list, which the arbitrator needs so a missing verdict reads as a gap rather than an absence). - **One auditor prompt, three seats.** The resolver emits `auditor_prompt` **once** and @@ -230,7 +241,6 @@ prompts/ ``` biggiepockets.review - ├── codex.review ├── magi.audit.caspar ├── magi.audit.balthazar ├── magi.audit.melchior @@ -255,12 +265,13 @@ prompts/ Spans carrying `arm`, `arm_role`, `arm_agreement`, `experiment_verdict`, or `label_assignment` tags came from the earlier A/B setup and are not comparable to these; - exclude them when querying. + neither are spans carrying `codex_model` or a `codex.review` child, which came from a + pre-panel first pass that no longer runs. Exclude both when querying. **Registry operations:** - **Roll** — edit a prompt or shared-rule file; its content-derived `prompt_version` bumps. -- **Apply** — point `codex_prompt`, `auditor_prompt`, or `arbiter_prompt` at a different +- **Apply** — point `auditor_prompt` or `arbiter_prompt` at a different stored prompt (no version change). Callers tracking `@main` pick the change up on their next run. A caller that pins `uses:` to a tag or SHA needs BOTH that `@ref` and its `registry_ref` input bumped in lockstep — Apply owns that ref-bump explicitly. diff --git a/prompts/codex-first-pass.md b/prompts/codex-first-pass.md deleted file mode 100644 index a490f53..0000000 --- a/prompts/codex-first-pass.md +++ /dev/null @@ -1,38 +0,0 @@ -Perform a first-pass code review of this pull request and output a concise -markdown findings report. Your final message IS the report — it is captured -verbatim and handed to a second reviewer, so do not add conversational preamble. - -1. Read ticket.json. If "available" is true, treat its summary as the intended behavior and - its acceptance_criteria as GUIDELINES, not a hard contract — they describe the minimum the - ticket set out to achieve, not the ceiling on what counts as correct. Do not flag a change - just because it doesn't match the acceptance criteria verbatim: first judge whether the - change actually satisfies the ticket's intent, and give particular credit when the author - went beyond the literal criteria (see step 2 for how to weigh that against history and - context). Only treat a deviation as a blocker if it genuinely fails the ticket's purpose. - If the ticket is not available, do a diff-based review. -2. Review the diff in pr.diff. Do NOT rely on the hunks alone: use `git log` / `git show` on - the branch and grep the repository for callers and related tests to judge impact and - intent. Git history and the PR's context are usually more reliable evidence of what was - meant than the ticket's literal acceptance criteria: when a commit, the diff, or the - conversation shows the work was refined or extended beyond the AC in a sound way, treat - that as an improvement rather than scope creep. -3. Enforce task completeness and reject incomplete tasks, half-measures, placeholders, or - deferred work per these rules as blocking issues: - {{@prompts/_shared/completeness-rules.md}} -4. Enforce these BiggerPockets member-privacy rules and flag any violation with a - file/line reference: - {{@prompts/_shared/privacy-rules.md}} -5. {{@prompts/_shared/migration-data-rule.md}} -6. Check the diff for batch/task performance hot spots per these rules, and flag each - genuine one with a file/line reference and a suggested fix: - {{@prompts/_shared/perf-rules.md}} -7. Check how the diff parses structured values, per these rules: - {{@prompts/_shared/parsing-rules.md}} -8. Check that in-app navigational links use React Router's Link rather than a raw `` - tag, per these rules: - {{@prompts/_shared/navigation-rules.md}} -9. Report concrete issues — bugs, regressions, security problems, member-privacy - violations, incomplete tasks/half-measures/placeholders/deferred work, and genuine misses - of the ticket's intent or clear scope creep — each with a file/line reference and a brief - rationale. Do not list "doesn't match acceptance criteria" as an issue by itself; only - raise it when the deviation harms the intent. If nothing is blocking, say so briefly. \ No newline at end of file diff --git a/prompts/magi-arbitrate.md b/prompts/magi-arbitrate.md index ea8d98a..c3de39a 100644 --- a/prompts/magi-arbitrate.md +++ b/prompts/magi-arbitrate.md @@ -11,7 +11,7 @@ request. arbitrate over the verdicts you do have — never silently treat a missing auditor as an approval. 2. Read the evidence the auditors worked from: pr.diff, ticket.json (its acceptance_criteria - are guidelines, not a hard contract), conversations.json, and codex-findings.md. + are guidelines, not a hard contract) and conversations.json. 3. Verify each distinct blocking finding against the code yourself. Use `git log` / `git show` on the branch and grep for callers and tests. Findings often overlap: treat two auditors describing the same defect at the same location as ONE finding raised twice, not diff --git a/prompts/magi-audit.md b/prompts/magi-audit.md index 7735d31..8431683 100644 --- a/prompts/magi-audit.md +++ b/prompts/magi-audit.md @@ -2,51 +2,48 @@ You are one of three independent auditors reviewing pull request #{{PR}}. Audit change on its own merits and reach your own verdict. You have no visibility into the other auditors and must not speculate about what they concluded — a separate arbitrator weighs all three verdicts afterwards. Your job is an honest, self-contained judgment, -not a guess at consensus. +not a guess at consensus. No one has reviewed this diff before you: every issue in your +verdict is one you found yourself. 1. Read ticket.json. If "available" is true, treat its summary and description as the intended behavior, and its acceptance_criteria (Atlassian Document Format JSON) as GUIDELINES rather than a hard contract. The AC describe the minimum the ticket set out to achieve, not the ceiling on what counts as correct: don't reject a change merely for not matching them verbatim. Judge whether the change satisfies the ticket's intent, and weigh - git history and the PR discussion (steps 3-4) as more authoritative evidence of what was + git history and the PR discussion (steps 2-3) as more authoritative evidence of what was actually meant. -2. Read the first-pass findings in codex-findings.md (if missing or empty, proceed with - your own review). These are leads to verify, not conclusions to adopt: confirm each one - against the code yourself and discard the false positives. -3. Read conversations.json: the PR's existing discussion (issue_comments, review_comments +2. Read conversations.json: the PR's existing discussion (issue_comments, review_comments with file/line, and prior reviews). Factor this context into your review: respect decisions already settled in the thread, don't re-raise concerns the author has already addressed or that were explicitly accepted, and weigh unresolved objections raised by human reviewers. Treat the conversation as more reliable than the ticket's literal acceptance criteria: if the thread shows the work was refined or explicitly accepted beyond the AC in a sound way, honor that. -4. Independently review the diff in pr.diff. Do NOT rely on the hunks alone: use `git log` / +3. Review the diff in pr.diff. Do NOT rely on the hunks alone: use `git log` / `git show` on the branch to inspect commit history and grep for callers and related tests to judge impact. When the diff adds or changes image assets, check that they are provided at retina quality (e.g. a 2x asset or an `srcset`/`@2x` variant); flag raster images that ship only at 1x where a high-DPI version is expected. {{@prompts/_shared/migration-data-rule.md}} -5. Enforce task completeness and reject incomplete tasks, half-measures, placeholders, or +4. Enforce task completeness and reject incomplete tasks, half-measures, placeholders, or deferred work per these rules as blocking issues: {{@prompts/_shared/completeness-rules.md}} -6. Enforce these BiggerPockets member-privacy rules and treat a genuine violation as a +5. Enforce these BiggerPockets member-privacy rules and treat a genuine violation as a blocking issue: {{@prompts/_shared/privacy-rules.md}} -7. Check for batch/task performance hot spots per these rules — an N+1 or O(n^2)+ +6. Check for batch/task performance hot spots per these rules — an N+1 or O(n^2)+ pattern on a full-table #process/#collection is a genuine finding, not a nitpick; flag it with a suggested fix: {{@prompts/_shared/perf-rules.md}} -8. Check how the diff parses structured values, per these rules: +7. Check how the diff parses structured values, per these rules: {{@prompts/_shared/parsing-rules.md}} -9. Check that in-app navigational links use React Router's Link rather than a raw `` +8. Check that in-app navigational links use React Router's Link rather than a raw `` tag, per these rules: {{@prompts/_shared/navigation-rules.md}} -10. Add any genuine issues the first pass missed, and (when a ticket is available) judge - genuine misses of the ticket's intent or clear scope creep — but give credit when the - author went beyond the literal acceptance criteria in a sound way rather than flagging it - as non-compliant. -11. Decide ONE verdict. Be pragmatic: use "request_changes" only when there is at least one +9. When a ticket is available, judge genuine misses of the ticket's intent or clear scope + creep — but give credit when the author went beyond the literal acceptance criteria in a + sound way rather than flagging it as non-compliant. +10. Decide ONE verdict. Be pragmatic: use "request_changes" only when there is at least one genuine, blocking issue (such as a bug, regression, privacy violation, half-finished task, placeholder, or deferred work); otherwise "approve". A change that exceeds the AC without breaking the ticket's intent is a reason to approve, not to block. @@ -67,9 +64,8 @@ Write a file named verdict.json in the current working directory with EXACTLY th - "confidence" is how sure you are of your own verdict. Say "low" when the diff's impact turned on code you could not inspect, and reserve "high" for a verdict you could defend line by line. The arbitrator weighs this, so an inflated confidence corrupts the panel. -- In the summary, state your rationale, which first-pass findings you confirmed or rejected - and why, anything you added, how the change measures against the ticket, and how the - existing PR discussion informed your decision. +- In the summary, state your rationale, how the change measures against the ticket, and how + the existing PR discussion informed your decision. Write the file even if you found nothing: an approve with an empty blocking_findings list is a complete verdict. Do not write any other file, and do not post a review or comment — the diff --git a/prompts/registry.json b/prompts/registry.json index 5a84b08..f219934 100644 --- a/prompts/registry.json +++ b/prompts/registry.json @@ -7,6 +7,11 @@ "//": "another's verdict: the audit outputs are uploaded per seat and are only ever", "//": "downloaded by the arbitrator job that runs after all three finish.", "//": "", + "//": "Nothing reviews the diff before the panel does. The only work that happens ahead", + "//": "of the auditors is gathering context — the diff, the JIRA ticket and its", + "//": "acceptance criteria, and the PR discussion — so no finding is planted in front of", + "//": "three auditors whose independence is the point.", + "//": "", "//": "The arbitrator then reads all three verdicts, weighs them, and issues the ONE", "//": "verdict that gets posted as the approve / request-changes review. It is the only", "//": "stage whose output reaches the pull request.", @@ -23,11 +28,10 @@ "//": "derived from file content by scripts/resolve-prompts.sh, never listed here.", "//": "", "//": "Operations: Roll = edit a prompt file (its derived version bumps). Apply = point", - "//": "codex_prompt, auditor_prompt, or arbiter_prompt at a different prompt file (no", - "//": "version change). Every consuming repo resolves this one file, so a change here", - "//": "changes them all at once.", - "version": 3, - "codex_prompt": "codex-first-pass", + "//": "auditor_prompt or arbiter_prompt at a different prompt file (no version change).", + "//": "Every consuming repo resolves this one file, so a change here changes them all at", + "//": "once.", + "version": 4, "auditor_prompt": "magi-audit", "arbiter_prompt": "magi-arbitrate", "auditors": ["caspar", "balthazar", "melchior"] diff --git a/scripts/resolve-prompts.sh b/scripts/resolve-prompts.sh index 3eb1c75..a8abb56 100755 --- a/scripts/resolve-prompts.sh +++ b/scripts/resolve-prompts.sh @@ -1,11 +1,12 @@ #!/usr/bin/env bash # resolve-prompts.sh — resolve the BiggiePockets review prompts from the registry. # -# prompts/registry.json declares the codex prompt, the auditor prompt, the arbitrator -# prompt, and the panel seats. A review runs the auditor prompt once per seat (caspar, -# balthazar, melchior) in independent jobs, then the arbitrator prompt once over the -# three verdicts those jobs produced. This script emits every prompt each review needs; -# the workflow decides which model runs in which seat. +# prompts/registry.json declares the auditor prompt, the arbitrator prompt, and the +# panel seats. A review runs the auditor prompt once per seat (caspar, balthazar, +# melchior) in independent jobs, then the arbitrator prompt once over the three verdicts +# those jobs produced. Those are the only two prompts a review runs — nothing reviews the +# diff ahead of the panel. This script emits both; the workflow decides which model runs +# in which seat. # # The auditor prompt is emitted ONCE and every seat runs that same text. Nothing in it # is parameterized by seat, so the three audits differ only by model and by the @@ -13,7 +14,7 @@ # # Shared rule blocks (privacy, migration-data, perf) are stored once in # prompts/_shared/ and injected into every prompt that references them via {{@path}} -# markers, so the Codex and auditor prompts can never drift out of sync. +# markers, so one edit updates every prompt that enforces that rule. # # The resolved text substitutes late-binding placeholders used for provenance only: # {{PR}}, {{PROMPT_NAME}}, {{PROMPT_VERSION}}, {{AUDITORS}} @@ -30,9 +31,9 @@ # review quality to the exact prompt text that produced it. # # Inputs (env): REGISTRY_DIR, PR. -# Outputs: codex_prompt{,_name,_version,_template}; auditor_prompt{,_name,_version,_template}; -# arbiter_prompt{,_name,_version,_template}; auditors (JSON array of seat names, which the -# workflow feeds straight into its fan-out matrix); auditor_count. +# Outputs: auditor_prompt{,_name,_version,_template}; arbiter_prompt{,_name,_version,_template}; +# auditors (JSON array of seat names, which the workflow feeds straight into its fan-out +# matrix); auditor_count. set -euo pipefail @@ -46,14 +47,13 @@ if [ ! -f "$REGISTRY_FILE" ]; then fi MAP_VERSION=$(jq -r '.version // empty' "$REGISTRY_FILE") -CODEX_NAME=$(jq -r '.codex_prompt // empty' "$REGISTRY_FILE") AUDITOR_NAME=$(jq -r '.auditor_prompt // empty' "$REGISTRY_FILE") ARBITER_NAME=$(jq -r '.arbiter_prompt // empty' "$REGISTRY_FILE") # Validate the config so a bad edit is caught at review time instead of silently # running a stale prompt or an undersized panel. : "${MAP_VERSION:?registry.json missing version}" -for field in codex_prompt auditor_prompt arbiter_prompt; do +for field in auditor_prompt arbiter_prompt; do name=$(jq -r --arg f "$field" '.[$f] // empty' "$REGISTRY_FILE") if [ -z "$name" ]; then echo "::error::registry.json is missing $field" >&2 @@ -173,7 +173,6 @@ write_output() { # name value — multiline-safe via GITHUB_OUTPUT heredoc; fixe printf '%s<<_BIGGIEPOCKETS_PROMPT_EOF_\n%s\n_BIGGIEPOCKETS_PROMPT_EOF_\n' "$name" "$value" >> "$GITHUB_OUTPUT" } -emit_prompt "codex" "$CODEX_NAME" emit_prompt "auditor" "$AUDITOR_NAME" emit_prompt "arbiter" "$ARBITER_NAME" @@ -181,4 +180,4 @@ emit_prompt "arbiter" "$ARBITER_NAME" write_output "auditors" "$(jq -c '.auditors' "$REGISTRY_FILE")" write_output "auditor_count" "${#AUDITORS[@]}" -echo "Resolved v$MAP_VERSION prompts: codex=$CODEX_NAME auditor=$AUDITOR_NAME arbiter=$ARBITER_NAME panel=[$AUDITOR_LIST]" +echo "Resolved v$MAP_VERSION prompts: auditor=$AUDITOR_NAME arbiter=$ARBITER_NAME panel=[$AUDITOR_LIST]" From e748272075b7773f1c32f460932f4e7b673050ab Mon Sep 17 00:00:00 2001 From: Aaron Breckenridge Date: Fri, 28 Aug 2026 12:20:02 -0500 Subject: [PATCH 4/7] Require an explicit model per seat and draw the pipeline as a diagram MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drops MAGI_MODEL_DEFAULT. A seat with no MAGI_MODEL_ now fails its matrix leg with an explicit error instead of falling back: a seat quietly running the same model as another seat is a panel that has lost a voice while still reporting three verdicts, which is worse than a leg that fails and says why. Adding a seat is deliberately a two-part change — registry entry plus model. Replaces the README's ASCII span tree with a mermaid diagram of the pipeline, which is the more useful view now that stage 1 runs no model and has no span of its own. --- .github/workflows/biggiepockets-review.yml | 18 ++++---- README.md | 49 +++++++++++++++------- 2 files changed, 44 insertions(+), 23 deletions(-) diff --git a/.github/workflows/biggiepockets-review.yml b/.github/workflows/biggiepockets-review.yml index 2d82125..0e39280 100644 --- a/.github/workflows/biggiepockets-review.yml +++ b/.github/workflows/biggiepockets-review.yml @@ -32,8 +32,10 @@ name: BiggiePockets Code Review (reusable) # MAGI_MODEL_CASPAR caspar (default openai/gpt-5.6-luna) # MAGI_MODEL_BALTHAZAR balthazar (default google/gemini-3.7-flash) # MAGI_MODEL_MELCHIOR melchior (default z-ai/glm-5.3-flash) -# MAGI_MODEL_DEFAULT any seat with no default and no override of its own # ARBITER_MODEL arbitration (default openai/gpt-5.6-sol) +# Every seat needs its own entry. There is no fallback model on purpose: a seat quietly +# running the same model as another seat is a panel that has lost a voice while still +# reporting three verdicts, which is worse than a matrix leg that fails and says so. # The three seats run different models on purpose: the auditor prompt is identical, so # what a disagreement between seats measures is the models, and three runs of one model # would mostly agree trivially and leave the arbitrator nothing to weigh. @@ -259,12 +261,11 @@ jobs: PR: ${{ inputs.pr }} AUDITOR: ${{ matrix.auditor }} # One OpenRouter slug per seat, each overrideable by the matching repository - # variable. MAGI_MODEL_DEFAULT only catches a seat added to registry.json that has - # no entry here, so a new seat runs rather than failing the matrix. + # variable. A seat with no entry here has no model and fails its matrix leg — see + # the note in the header on why there is no fallback. MAGI_MODEL_CASPAR: ${{ vars.MAGI_MODEL_CASPAR || 'openai/gpt-5.6-luna' }} MAGI_MODEL_BALTHAZAR: ${{ vars.MAGI_MODEL_BALTHAZAR || 'google/gemini-3.7-flash' }} MAGI_MODEL_MELCHIOR: ${{ vars.MAGI_MODEL_MELCHIOR || 'z-ai/glm-5.3-flash' }} - MAGI_MODEL_DEFAULT: ${{ vars.MAGI_MODEL_DEFAULT || 'google/gemini-3.7-flash' }} steps: - name: Checkout reviewed PR head uses: actions/checkout@v4 @@ -299,16 +300,17 @@ jobs: run: registry/scripts/resolve-prompts.sh # Look the seat's model up by name rather than hard-coding a model per matrix leg, - # so adding a seat to registry.json needs no change here and an override is a - # repository variable rather than a workflow edit. + # so an override is a repository variable rather than a workflow edit. A seat with + # no MAGI_MODEL_ of its own fails here: adding a seat to registry.json is + # deliberately a two-part change, because the seat only earns its cost if it runs a + # model no other seat is running. - name: Select seat model id: seat run: | var="MAGI_MODEL_$(printf '%s' "$AUDITOR" | tr '[:lower:]-' '[:upper:]_')" model="${!var:-}" - if [ -z "$model" ]; then model="$MAGI_MODEL_DEFAULT"; fi if [ -z "$model" ]; then - echo "::error::No model resolved for auditor '$AUDITOR' (set $var or MAGI_MODEL_DEFAULT)" >&2 + echo "::error::Auditor '$AUDITOR' has no model: set $var as a repository variable, or give it a default in this workflow's audit job." >&2 exit 1 fi echo "model=$model" >> "$GITHUB_OUTPUT" diff --git a/README.md b/README.md index 8bd7408..e1cafbe 100644 --- a/README.md +++ b/README.md @@ -25,6 +25,26 @@ is exactly the failure a panel exists to avoid — so CI asserts no prompt reads pre-existing findings file and the registry declares no prompt but the auditor's and the arbitrator's. +```mermaid +flowchart LR + ctx["1 · context
diff · ticket + AC · discussion
(no model)"] + subgraph panel["2 · panel — one prompt, three separate jobs"] + direction TB + caspar["caspar
openai/gpt-5.6-luna"] + balthazar["balthazar
google/gemini-3.7-flash"] + melchior["melchior
z-ai/glm-5.3-flash"] + end + arb["3 · arbitrate
openai/gpt-5.6-sol"] + posted(["the one verdict the PR sees"]) + ctx --> caspar + ctx --> balthazar + ctx --> melchior + caspar --> arb + balthazar --> arb + melchior --> arb + arb --> posted +``` + The **BiggiePockets** service account then submits that `approve` / `request_changes` review on the PR. If the PR has no `BIG-XXXXX` key in its title (or the ticket can't be fetched), the review degrades gracefully to a diff-based review instead of failing. @@ -78,10 +98,14 @@ repo. An unset or empty variable keeps the default. Stage 1 runs no model, so it | melchior | `MAGI_MODEL_MELCHIOR` | `z-ai/glm-5.3-flash` | | Arbitration | `ARBITER_MODEL` | `openai/gpt-5.6-sol` | -`MAGI_MODEL_DEFAULT` (default `google/gemini-3.7-flash`) covers a seat added to -`prompts/registry.json` that has no entry of its own, so a new seat runs instead of failing -the matrix. Changing a seat's model never touches the prompt — the auditor prompt is -seat-agnostic by construction, and CI asserts it. +There is no fallback model. A seat listed in `prompts/registry.json` with no +`MAGI_MODEL_` of its own fails its matrix leg with an explicit error, so adding a seat +is deliberately a two-part change: registry entry plus model. A seat that quietly ran the +same model as another seat would be a panel that had lost a voice while still reporting three +verdicts — worse than a leg that fails and says why. + +Changing a seat's model never touches the prompt — the auditor prompt is seat-agnostic by +construction, and CI asserts it. The review logic lives centrally in this repo. Each consuming repo only adds a thin **caller** workflow that owns the triggers and gating and delegates to this one. @@ -237,15 +261,9 @@ prompts/ volume, latency, tokens, and a version diff per prompt, and any span can be replayed in the Playground with its exact template and variables. A version starts when the prompt text changes (a Roll), since `version` is the same content hash reported as a tag. -- **Datadog.** Each review is one trace, one span per stage and one per seat: - - ``` - biggiepockets.review - ├── magi.audit.caspar - ├── magi.audit.balthazar - ├── magi.audit.melchior - └── magi.arbitrate - ``` +- **Datadog.** Each review is one trace: a `biggiepockets.review` root with one + `magi.audit.` child per seat and one `magi.arbitrate` child — the shape of the + diagram above, minus stage 1, which runs no model and so has no LLM span. A seat's own verdict, confidence, model, and blocking-finding count live on that seat's span and on `auditor_verdict.` / `auditor_confidence.` / @@ -275,8 +293,9 @@ prompts/ stored prompt (no version change). Callers tracking `@main` pick the change up on their next run. A caller that pins `uses:` to a tag or SHA needs BOTH that `@ref` and its `registry_ref` input bumped in lockstep — Apply owns that ref-bump explicitly. -- **Seat** — add or remove a name in `auditors`, and give a new seat a `MAGI_MODEL_` - default in the workflow (without one it falls back to `MAGI_MODEL_DEFAULT`). +- **Seat** — add or remove a name in `auditors`. A new seat also needs a + `MAGI_MODEL_` default in the workflow (or that repository variable set); without + one its matrix leg fails rather than silently doubling up on another seat's model. A `validate-prompts.yml` workflow guards the registry: it fails a PR if a template has dangling includes, `registry.json` references a missing prompt, the auditor prompt names a From 4836250a20e8c2129dfe66d2584ccb38f0476ee1 Mon Sep 17 00:00:00 2001 From: Aaron Breckenridge Date: Fri, 28 Aug 2026 12:41:50 -0500 Subject: [PATCH 5/7] Give each seat its own job and stage 1 the whole context to gather MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes, both about what "independent" and "context" actually mean here. Each wise man is now a job of its own — `caspar`, `balthazar`, `melchior` — written out rather than fanned out from a matrix. A seat is an independent unit end to end: its own runner, its own model, its own status check, re-runnable on its own. Everything seat-specific lives in the job's `env`, which leaves the three `steps:` blocks byte-identical; that is load-bearing, since a split between seats is only evidence about the models if the harness around them is identical, and three copies of a block drift one seat at a time. CI now diffs those blocks and fails if they stop matching, and fails if the set of seat jobs stops matching the roster in registry.json. Stage 1 now gathers everything the panel judges against and hands it over as artifacts: the diff, the PR itself (body, labels, commit messages, touched files), the JIRA ticket and its acceptance criteria, that ticket's epic, the PR discussion, and the design references found in all of those texts — plus context-manifest.json, which records what was gathered and what was not. The manifest is the point of the whole stage. Without it an auditor cannot tell "there is no epic for this work" from "the epic fetch failed", and both read as silence — which is how a review ends up judging intent against nothing and not saying so. Every source degrades to {"available": false, "reason": ...} rather than failing the review, and the prompts require an auditor to name the context it did not have. Design links are recorded, never followed: the panel has no Figma credentials, and a code review has no reason to pull design or member-facing content into a build artifact. What a recorded link buys an auditor is knowing a design exists for this work, and the prompts forbid treating a reference it cannot open as a defect. No handoff file carries an author, assignee or reporter — the panel judges the change, not who wrote it — with the PR discussion the one exception, where attribution is what makes an unresolved human objection legible. Breaking for callers with required status checks: the seats were reported as `audit (caspar)` and friends; they are now `caspar`, `balthazar`, `melchior`. --- .github/workflows/biggiepockets-review.yml | 567 ++++++++++++++++++--- .github/workflows/validate-prompts.yml | 62 ++- README.md | 96 +++- prompts/magi-arbitrate.md | 7 +- prompts/magi-audit.md | 48 +- prompts/registry.json | 21 +- scripts/resolve-prompts.sh | 7 +- 7 files changed, 679 insertions(+), 129 deletions(-) diff --git a/.github/workflows/biggiepockets-review.yml b/.github/workflows/biggiepockets-review.yml index 0e39280..e767660 100644 --- a/.github/workflows/biggiepockets-review.yml +++ b/.github/workflows/biggiepockets-review.yml @@ -4,16 +4,20 @@ name: BiggiePockets Code Review (reusable) # service account submitting the resulting approve / request-changes decision. # # A review runs in three stages: -# 1. Context — gathers what the panel judges against: the diff, the PR's JIRA ticket -# and its acceptance criteria, and the PR discussion. No model reviews -# the code here. Nothing reads the diff ahead of the panel, so no -# finding is planted in front of the auditors whose independence is the -# whole point of the panel. +# 1. Context — gathers everything the panel judges against and uploads it as one +# immutable handoff: the diff, the PR itself (body, labels, commits, +# touched files), the JIRA ticket and its acceptance criteria, that +# ticket's epic, the PR discussion, and any design references found in +# those texts. Whatever isn't available is recorded as unavailable, in +# context-manifest.json, so an auditor can tell "there is no epic" from +# "the epic fetch failed". No model reviews the code here: nothing +# reads the diff ahead of the panel, so no finding is planted in front +# of the auditors whose independence is the whole point. # 2. Panel — three auditors (caspar, balthazar, melchior) each audit the change -# INDEPENDENTLY, in separate jobs, running the SAME auditor prompt, -# and each writes its own verdict. No auditor can see another's -# verdict: a seat's output is uploaded per seat and downloaded only by -# stage 3. +# INDEPENDENTLY, each in a job of its own named for the seat, all +# running the SAME auditor prompt, and each writing its own verdict. +# No auditor can see another's verdict: a seat's output is uploaded per +# seat and downloaded only by stage 3. # 3. Arbitration — one arbitrator reads all three verdicts, verifies the blocking # findings against the code, and issues the ONE verdict that is posted. # It is the only stage whose output reaches the pull request. @@ -33,9 +37,10 @@ name: BiggiePockets Code Review (reusable) # MAGI_MODEL_BALTHAZAR balthazar (default google/gemini-3.7-flash) # MAGI_MODEL_MELCHIOR melchior (default z-ai/glm-5.3-flash) # ARBITER_MODEL arbitration (default openai/gpt-5.6-sol) -# Every seat needs its own entry. There is no fallback model on purpose: a seat quietly -# running the same model as another seat is a panel that has lost a voice while still -# reporting three verdicts, which is worse than a matrix leg that fails and says so. +# Every seat carries its own model in its own job. There is no shared fallback on +# purpose: a seat quietly running the same model as another seat is a panel that has lost +# a voice while still reporting three verdicts, which is worse than a seat that fails and +# says so. # The three seats run different models on purpose: the auditor prompt is identical, so # what a disagreement between seats measures is the models, and three runs of one model # would mostly agree trivially and leave the arbitrator nothing to weigh. @@ -78,13 +83,14 @@ jobs: # same commit and context. # # NOTE for callers: this job was previously named `codex`. A repo that lists it as a - # required status check must switch that check to `context`. + # required status check must switch that check to `context`. The panel's checks were + # likewise `audit (caspar)` and friends when the seats were matrix legs; they are now + # `caspar`, `balthazar` and `melchior`. context: runs-on: ubuntu-latest outputs: head_sha: ${{ steps.pr_head.outputs.sha }} registry_sha: ${{ steps.registry_head.outputs.sha }} - auditors: ${{ steps.resolve.outputs.auditors }} permissions: contents: read pull-requests: read @@ -131,16 +137,40 @@ jobs: - name: Record review start time run: echo "REVIEW_START_NS=$(date +%s%N)" >> "$GITHUB_ENV" - # Resolve PR context (base ref and title) from the API. - - name: Resolve PR context + # Everything from here to the upload is context gathering. Each source writes one + # artifact file and nothing more: no step reads another's output to form a view of + # the code. A source that isn't there degrades to {"available": false, "reason": ...} + # instead of failing the review — a PR with no ticket, no epic and no designs is + # still reviewable, just with less to judge intent against. + # + # No artifact carries an author, assignee or reporter. The panel judges the change, + # not who wrote it, and leaving identities out keeps them off the handoff and out of + # telemetry. The PR discussion is the one exception: attribution is what makes + # "a human reviewer objected and it was never resolved" legible. + - name: Gather PR metadata env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | - base=$(gh pr view "$PR" --repo "$GITHUB_REPOSITORY" --json baseRefName -q .baseRefName) - title=$(gh pr view "$PR" --repo "$GITHUB_REPOSITORY" --json title -q .title) + gh pr view "$PR" --repo "$GITHUB_REPOSITORY" \ + --json number,title,body,baseRefName,headRefName,isDraft,labels,additions,deletions,changedFiles,commits,files \ + > pr-raw.json + + jq '{ + available: true, + number, title, body, + base_ref: .baseRefName, + head_ref: .headRefName, + draft: .isDraft, + labels: [.labels[].name], + additions, deletions, + changed_files: .changedFiles, + commits: [.commits[] | {message: .messageHeadline, body: .messageBody}], + files: [.files[] | {path, additions, deletions}] + }' pr-raw.json > pr.json + + base=$(jq -r '.base_ref' pr.json) echo "BASE_REF=$base" >> "$GITHUB_ENV" - echo "PR_TITLE=$title" >> "$GITHUB_ENV" - echo "Reviewing PR #$PR (base $base): $title" + echo "Reviewing PR #$PR (base $base): $(jq -r '.title' pr.json)" - name: Compute PR diff run: | @@ -150,13 +180,14 @@ jobs: # Anchor the review on the work's intent. Always writes ticket.json: either the # ticket fields, or {"available": false} so a missing/unfetchable ticket degrades - # to a diff-based review instead of failing the review. + # to a diff-based review instead of failing the review. Also records the parent + # key, which is what the epic step below fetches. - name: Fetch JIRA ticket for intent env: JIRA_EMAIL: ${{ secrets.JIRA_EMAIL }} JIRA_API_TOKEN: ${{ secrets.JIRA_API_TOKEN }} run: | - key=$(printf '%s' "$PR_TITLE" | grep -oE 'BIG-[0-9]+' | head -1 || true) + key=$(jq -r '.title' pr.json | grep -oE 'BIG-[0-9]+' | head -1 || true) if [ -z "$key" ]; then echo '{"available": false, "reason": "no BIG-XXXXX key in PR title"}' > ticket.json echo "No JIRA ticket key in PR title; falling back to diff-based review." @@ -165,18 +196,72 @@ jobs: http=$(curl -sS -w '%{http_code}' -o ticket-raw.json \ -u "$JIRA_EMAIL:$JIRA_API_TOKEN" -H 'Accept: application/json' \ - "https://biggerpockets.atlassian.net/rest/api/3/issue/$key?fields=summary,description,customfield_11557") \ + "https://biggerpockets.atlassian.net/rest/api/3/issue/$key?fields=summary,description,customfield_11557,status,issuetype,labels,parent,attachment") \ || http="000" if [ "$http" = "200" ]; then - jq --arg key "$key" '{available: true, key: $key, summary: .fields.summary, description: .fields.description, acceptance_criteria: .fields.customfield_11557}' \ - ticket-raw.json > ticket.json + jq --arg key "$key" '{ + available: true, + key: $key, + summary: .fields.summary, + description: .fields.description, + acceptance_criteria: .fields.customfield_11557, + status: .fields.status.name, + issue_type: .fields.issuetype.name, + labels: .fields.labels, + parent: (if .fields.parent then {key: .fields.parent.key, summary: .fields.parent.fields.summary, type: .fields.parent.fields.issuetype.name} else null end) + }' ticket-raw.json > ticket.json + echo "PARENT_KEY=$(jq -r '.parent.key // ""' ticket.json)" >> "$GITHUB_ENV" echo "Wrote ticket.json for $key" else echo "{\"available\": false, \"reason\": \"fetch failed (HTTP $http) for $key\"}" > ticket.json echo "Could not fetch $key (HTTP $http); falling back to diff-based review." fi + # The epic is where the wider goal usually lives: a ticket's own description is + # often a slice of work whose purpose only makes sense against the epic it belongs + # to. Fetched separately because a ticket can have no parent, or a parent the token + # can't read, and neither is a reason to fail a review. + - name: Fetch the ticket's epic for wider intent + env: + JIRA_EMAIL: ${{ secrets.JIRA_EMAIL }} + JIRA_API_TOKEN: ${{ secrets.JIRA_API_TOKEN }} + run: | + # Distinguish the three ways there can be no epic. "No epic" and "I could not + # read the epic" are different pieces of evidence, and an auditor that cannot + # tell them apart cannot say which one it judged without. + if [ "$(jq -r '.available' ticket.json)" != "true" ]; then + echo '{"available": false, "reason": "no ticket was available, so no epic to fetch"}' > epic.json + echo "No ticket, so no epic to fetch." + exit 0 + fi + if [ -z "${PARENT_KEY:-}" ]; then + echo '{"available": false, "reason": "ticket has no parent epic"}' > epic.json + echo "No parent epic to fetch." + exit 0 + fi + + http=$(curl -sS -w '%{http_code}' -o epic-raw.json \ + -u "$JIRA_EMAIL:$JIRA_API_TOKEN" -H 'Accept: application/json' \ + "https://biggerpockets.atlassian.net/rest/api/3/issue/$PARENT_KEY?fields=summary,description,issuetype,status,labels") \ + || http="000" + + if [ "$http" = "200" ]; then + jq --arg key "$PARENT_KEY" '{ + available: true, + key: $key, + summary: .fields.summary, + description: .fields.description, + issue_type: .fields.issuetype.name, + status: .fields.status.name, + labels: .fields.labels + }' epic-raw.json > epic.json + echo "Wrote epic.json for $PARENT_KEY" + else + echo "{\"available\": false, \"reason\": \"fetch failed (HTTP $http) for $PARENT_KEY\"}" > epic.json + echo "Could not fetch parent $PARENT_KEY (HTTP $http)." + fi + # Gather the existing PR discussion so the review accounts for context that lives # in conversation rather than the diff: general comments, inline review threads, and # prior review bodies. Always writes conversations.json (empty arrays if none). @@ -202,18 +287,115 @@ jobs: > conversations.json echo "Wrote conversations.json ($(jq '.issue_comments | length' conversations.json) comments, $(jq '.review_comments | length' conversations.json) review comments, $(jq '.reviews | length' conversations.json) reviews)" + # Design references, harvested from the text already gathered above — the PR body + # and commit messages, the ticket and epic (description and acceptance criteria + # included, since a Figma link is usually pasted into one of them), and the + # discussion — plus the ticket's own attachments. + # + # Links are RECORDED, never followed. The panel has no Figma credentials, and + # pulling design or member-facing content into a review artifact is not something a + # code review needs. What a recorded link buys the auditors is the knowledge that a + # design exists for this work, which is the difference between "no design was + # specified" and "a design was specified and I can't see it". + - name: Collect design references + run: | + # Every URL appearing anywhere in one of the context files. `..|strings` walks + # the whole document, which is what reaches Atlassian Document Format nodes + # without having to know their shape. + # `|| true` because "this file contains no URLs" is the ordinary case, and grep + # exits 1 on no match — without it a ticket with no links would fail the step. + urls_from() { # file source-label + [ -f "$1" ] || return 0 + jq -r '[.. | strings] | .[]' "$1" 2>/dev/null \ + | grep -oE 'https?://[^][ )"'"'"'<>,]+' \ + | sed 's/[.,;:)]*$//' \ + | sort -u \ + | jq -R --arg source "$2" '{url: ., source: $source}' || true + } + + { urls_from pr.json pr + urls_from ticket.json ticket + urls_from epic.json epic + urls_from conversations.json discussion + } | jq -s '.' > urls.json + + if [ -f ticket-raw.json ]; then + jq '[.fields.attachment // [] | .[] | {filename, mime_type: .mimeType, size, url: .content, source: "ticket"}]' \ + ticket-raw.json > attachments.json + else + echo '[]' > attachments.json + fi + + # Keep only URLs that are plausibly a design artifact. An unclassified link is + # dropped rather than listed: a designs.json full of Jira and CI links would + # bury the one Figma URL that matters. + jq -n \ + --slurpfile urls urls.json \ + --slurpfile attachments attachments.json \ + ' + def kind: + if test("figma\\.com") then "figma" + elif test("zeplin\\.io|invisionapp\\.com|sketch\\.com|miro\\.com|framer\\.com|abstract\\.com") then "design_tool" + elif test("loom\\.com|youtube\\.com|youtu\\.be|vimeo\\.com") then "walkthrough_video" + elif test("user-attachments|user-images\\.githubusercontent\\.com") then "screenshot" + elif test("\\.(png|jpe?g|gif|webp|svg|pdf)([?#]|$)") then "image" + else empty end; + # One entry per URL, listing every place it appeared: the same Figma link is + # usually in the ticket AND the PR body AND the discussion, and three copies + # of one link say nothing that one link with three sources does not. + ($urls[0] + | map(. + {kind: (.url | kind)}) + | group_by(.url) + | map({url: .[0].url, kind: .[0].kind, sources: (map(.source) | unique)}) + ) as $links + | ($attachments[0]) as $att + | { + available: ((($links | length) + ($att | length)) > 0), + links: $links, + attachments: $att, + note: "Links and attachments are recorded, not fetched: this review has no credentials for design tools. Treat a reference as evidence that a design exists for this work, not as something you can open, and never treat an unopenable reference as a defect." + }' > designs.json + echo "Wrote designs.json ($(jq '.links | length' designs.json) link(s), $(jq '.attachments | length' designs.json) attachment(s))" + + # One file that says what the panel actually got. Without it an auditor cannot tell + # "there is no epic for this work" from "the epic fetch failed", and both read as + # silence — which is how a review ends up judging intent against nothing and not + # saying so. + - name: Write the context manifest + run: | + jq -n \ + --arg head_sha "${{ steps.pr_head.outputs.sha }}" \ + --arg base_ref "$BASE_REF" \ + --argjson diff_lines "$(wc -l < pr.diff | tr -d " ")" \ + --slurpfile pr pr.json \ + --slurpfile ticket ticket.json \ + --slurpfile epic epic.json \ + --slurpfile conversations conversations.json \ + --slurpfile designs designs.json \ + '{ + head_sha: $head_sha, + base_ref: $base_ref, + diff: {file: "pr.diff", lines: $diff_lines}, + pr: {file: "pr.json", available: true, commits: ($pr[0].commits | length), files: ($pr[0].files | length)}, + ticket: {file: "ticket.json", available: $ticket[0].available, key: ($ticket[0].key // null), reason: ($ticket[0].reason // null), has_acceptance_criteria: ((($ticket[0].acceptance_criteria // {}) | [.. | objects | select(.type == "text") | .text] | length) > 0)}, + epic: {file: "epic.json", available: $epic[0].available, key: ($epic[0].key // null), reason: ($epic[0].reason // null)}, + conversations: {file: "conversations.json", issue_comments: ($conversations[0].issue_comments | length), review_comments: ($conversations[0].review_comments | length), reviews: ($conversations[0].reviews | length)}, + designs: {file: "designs.json", available: $designs[0].available, links: ($designs[0].links | length), attachments: ($designs[0].attachments | length)} + }' > context-manifest.json + cat context-manifest.json + # Resolve the prompts from the registry — the single auditor prompt every seat runs # and the arbitrator prompt — each with its shared-rule blocks expanded and a - # content-derived prompt_version. Also emits the panel seats, which drive the audit - # job's fan-out matrix. + # content-derived prompt_version. Also emits the panel seats, which the arbitrator + # uses to know how many verdicts to expect. - name: Resolve review prompts id: resolve env: REGISTRY_DIR: registry run: registry/scripts/resolve-prompts.sh - # Persist the handoff. Every auditor consumes the same diff and ticket/discussion - # context, on every retry. + # Persist the handoff. Every auditor consumes the same diff, ticket, epic, + # discussion and design references, on every retry. # No `if: always()` on this or the upload: a stage that failed part-way through # must not hand the panel a half-built context. Fail here and the whole review # stops, which is the correct outcome. @@ -225,47 +407,55 @@ jobs: echo "CONTEXT_END_NS=$(date +%s%N)" } > review-context.env + # The raw API responses are deliberately NOT uploaded: they carry fields nobody + # reviews (assignee, reporter, watchers) and the reshaped files above are what the + # prompts are written against. - name: Upload review handoff uses: actions/upload-artifact@v4 with: name: review-context-${{ github.run_id }} path: | pr.diff + pr.json ticket.json + epic.json conversations.json + designs.json + context-manifest.json review-context.env if-no-files-found: error retention-days: 7 overwrite: true - # Stage 2. One job per panel seat, fanned out over the seats the registry declares. - # Separate jobs are what make the audits independent: each runs on its own runner, sees - # only the stage-1 handoff, and writes only its own verdict. Nothing an auditor can read - # contains another auditor's output. + # Stage 2. Three wise men, three jobs, one prompt. # - # fail-fast is off so one seat's failure doesn't cancel the others mid-audit, and the - # model step is continue-on-error so a rate-limited seat still leaves a recorded status - # for the arbitrator. "Re-run failed jobs" re-runs only the seats that failed. - audit: + # Each seat is its own job, written out rather than fanned out from a matrix, so a seat + # is an independent unit end to end: its own runner, its own model, its own status check + # (`caspar`, `balthazar`, `melchior`), re-runnable on its own without touching the + # others. The only thing a seat can read is stage 1's handoff — nothing available to an + # auditor contains another auditor's output, which is what makes the three verdicts + # independent rather than merely concurrent. + # + # The three `steps:` blocks below are BYTE-IDENTICAL on purpose. Everything that differs + # between seats lives in the `env:` above them, so what a disagreement between seats + # measures is the models — a difference in harness would corrupt that comparison, and + # three copies of a block is exactly the kind of thing that drifts. Validation of that + # invariant is not left to reviewers: .github/workflows/validate-prompts.yml fails if + # the blocks stop matching, or if the set of seat jobs stops matching the registry. + # + # The model step is continue-on-error so a rate-limited seat still records a status for + # the arbitrator instead of taking the whole review down with it. + caspar: needs: context runs-on: ubuntu-latest - strategy: - fail-fast: false - matrix: - auditor: ${{ fromJSON(needs.context.outputs.auditors) }} permissions: contents: read pull-requests: read id-token: write # claude-code-action authenticates via OIDC env: PR: ${{ inputs.pr }} - AUDITOR: ${{ matrix.auditor }} - # One OpenRouter slug per seat, each overrideable by the matching repository - # variable. A seat with no entry here has no model and fails its matrix leg — see - # the note in the header on why there is no fallback. - MAGI_MODEL_CASPAR: ${{ vars.MAGI_MODEL_CASPAR || 'openai/gpt-5.6-luna' }} - MAGI_MODEL_BALTHAZAR: ${{ vars.MAGI_MODEL_BALTHAZAR || 'google/gemini-3.7-flash' }} - MAGI_MODEL_MELCHIOR: ${{ vars.MAGI_MODEL_MELCHIOR || 'z-ai/glm-5.3-flash' }} + AUDITOR: caspar + AUDITOR_MODEL: ${{ vars.MAGI_MODEL_CASPAR || 'openai/gpt-5.6-luna' }} steps: - name: Checkout reviewed PR head uses: actions/checkout@v4 @@ -282,6 +472,8 @@ jobs: persist-credentials: false path: registry + # The whole of stage 1: the diff, the PR, the ticket, its epic, the discussion, the + # design references, and the manifest saying which of those were available. - name: Download review handoff uses: actions/download-artifact@v4 with: @@ -299,29 +491,19 @@ jobs: REGISTRY_DIR: registry run: registry/scripts/resolve-prompts.sh - # Look the seat's model up by name rather than hard-coding a model per matrix leg, - # so an override is a repository variable rather than a workflow edit. A seat with - # no MAGI_MODEL_ of its own fails here: adding a seat to registry.json is - # deliberately a two-part change, because the seat only earns its cost if it runs a - # model no other seat is running. - - name: Select seat model - id: seat + # A seat with no model is a lost voice, not a fallback — fail before the panel can + # report a verdict short of a seat it claimed to have. + - name: Confirm seat model run: | - var="MAGI_MODEL_$(printf '%s' "$AUDITOR" | tr '[:lower:]-' '[:upper:]_')" - model="${!var:-}" - if [ -z "$model" ]; then - echo "::error::Auditor '$AUDITOR' has no model: set $var as a repository variable, or give it a default in this workflow's audit job." >&2 + if [ -z "$AUDITOR_MODEL" ]; then + echo "::error::Auditor '$AUDITOR' has no model: set MAGI_MODEL_$(printf '%s' "$AUDITOR" | tr '[:lower:]-' '[:upper:]_') as a repository variable, or give this job a default." >&2 exit 1 fi - echo "model=$model" >> "$GITHUB_OUTPUT" - echo "Auditor $AUDITOR runs $model" + echo "Auditor $AUDITOR runs $AUDITOR_MODEL" - name: Record audit start time run: echo "AUDIT_START_NS=$(date +%s%N)" >> "$GITHUB_ENV" - # The auditor prompt is identical for every seat — the resolver emits it once and - # substitutes nothing seat-specific — so the three audits differ only by model and - # by the independent judgment of the run. - name: Audit independently id: audit continue-on-error: true @@ -334,7 +516,7 @@ jobs: # by the claude bot are rejected with "Workflow initiated by non-human # actor: claude (type: Bot). Add bot to allowed_bots list." allowed_bots: 'claude' - claude_args: '--allowedTools "Read,Grep,Glob,Bash,Write" --model ${{ steps.seat.outputs.model }}' + claude_args: '--allowedTools "Read,Grep,Glob,Bash,Write" --model ${{ env.AUDITOR_MODEL }}' prompt: ${{ steps.resolve.outputs.auditor_prompt }} env: ANTHROPIC_BASE_URL: "https://openrouter.ai/api" @@ -363,27 +545,270 @@ jobs: # Shell-safe seat name so the arbitrator can source every seat's file at once. key=$(printf '%s' "$AUDITOR" | tr '[:lower:]-' '[:upper:]_') { - echo "AUDIT_${key}_START_NS=$AUDIT_START_NS" + echo "AUDIT_${key}_START_NS=${AUDIT_START_NS:-$audit_end_ns}" echo "AUDIT_${key}_END_NS=$audit_end_ns" echo "AUDIT_${key}_STATUS=$status" - echo "AUDIT_${key}_MODEL=${{ steps.seat.outputs.model }}" + echo "AUDIT_${key}_MODEL=$AUDITOR_MODEL" } > "audit-$AUDITOR.env" - name: Upload seat verdict if: always() uses: actions/upload-artifact@v4 with: - name: audit-${{ matrix.auditor }}-${{ github.run_id }} + name: audit-${{ env.AUDITOR }}-${{ github.run_id }} path: | - audit-${{ matrix.auditor }}.json - audit-${{ matrix.auditor }}.env + audit-${{ env.AUDITOR }}.json + audit-${{ env.AUDITOR }}.env + if-no-files-found: error + retention-days: 7 + overwrite: true + + balthazar: + needs: context + runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: read + id-token: write # claude-code-action authenticates via OIDC + env: + PR: ${{ inputs.pr }} + AUDITOR: balthazar + AUDITOR_MODEL: ${{ vars.MAGI_MODEL_BALTHAZAR || 'google/gemini-3.7-flash' }} + steps: + - name: Checkout reviewed PR head + uses: actions/checkout@v4 + with: + ref: ${{ needs.context.outputs.head_sha }} + fetch-depth: 0 + + - name: Checkout prompt registry + uses: actions/checkout@v4 + with: + repository: BiggerPockets/.github + ref: ${{ needs.context.outputs.registry_sha }} + token: ${{ secrets.BIGGIEPOCKETS_PAT }} + persist-credentials: false + path: registry + + # The whole of stage 1: the diff, the PR, the ticket, its epic, the discussion, the + # design references, and the manifest saying which of those were available. + - name: Download review handoff + uses: actions/download-artifact@v4 + with: + name: review-context-${{ github.run_id }} + path: . + + - name: Restore review context + run: cat review-context.env >> "$GITHUB_ENV" + + # Resolve again from the same pinned registry ref. Prompt text stays out of job + # outputs (which are size-limited), while the artifact carries all model inputs. + - name: Resolve review prompts + id: resolve + env: + REGISTRY_DIR: registry + run: registry/scripts/resolve-prompts.sh + + # A seat with no model is a lost voice, not a fallback — fail before the panel can + # report a verdict short of a seat it claimed to have. + - name: Confirm seat model + run: | + if [ -z "$AUDITOR_MODEL" ]; then + echo "::error::Auditor '$AUDITOR' has no model: set MAGI_MODEL_$(printf '%s' "$AUDITOR" | tr '[:lower:]-' '[:upper:]_') as a repository variable, or give this job a default." >&2 + exit 1 + fi + echo "Auditor $AUDITOR runs $AUDITOR_MODEL" + + - name: Record audit start time + run: echo "AUDIT_START_NS=$(date +%s%N)" >> "$GITHUB_ENV" + + - name: Audit independently + id: audit + continue-on-error: true + uses: anthropics/claude-code-action@v1 + with: + # The action requires this input for launch validation. ANTHROPIC_AUTH_TOKEN + # below is what makes Claude Code send the OpenRouter key as a bearer token. + anthropic_api_key: ${{ secrets.OPENROUTER_API_KEY }} + # Allow the `claude` bot to trigger this review. Without this, PRs opened + # by the claude bot are rejected with "Workflow initiated by non-human + # actor: claude (type: Bot). Add bot to allowed_bots list." + allowed_bots: 'claude' + claude_args: '--allowedTools "Read,Grep,Glob,Bash,Write" --model ${{ env.AUDITOR_MODEL }}' + prompt: ${{ steps.resolve.outputs.auditor_prompt }} + env: + ANTHROPIC_BASE_URL: "https://openrouter.ai/api" + ANTHROPIC_AUTH_TOKEN: ${{ secrets.OPENROUTER_API_KEY }} + + # Name the verdict for its seat before it leaves this job. The arbitrator reads + # audit-.json, and a seat that produced no usable verdict must be visible as + # such rather than absent: the recorded status is what stops a failed seat from + # being mistaken for a silent approval. + - name: Record seat verdict + if: always() + run: | + audit_end_ns=$(date +%s%N) + status="ok" + [ "${{ steps.audit.outcome }}" != "success" ] && status="error" + + if [ -f verdict.json ] && jq -e '.verdict == "approve" or .verdict == "request_changes"' verdict.json >/dev/null 2>&1; then + mv verdict.json "audit-$AUDITOR.json" + echo "$AUDITOR: $(jq -r '.verdict' "audit-$AUDITOR.json") (confidence $(jq -r '.confidence // "unstated"' "audit-$AUDITOR.json"), $(jq -r '.blocking_findings // [] | length' "audit-$AUDITOR.json") blocking finding(s))" + else + status="error" + rm -f verdict.json + echo "::warning::Auditor $AUDITOR produced no valid verdict.json; the arbitrator will arbitrate without this seat." + fi + + # Shell-safe seat name so the arbitrator can source every seat's file at once. + key=$(printf '%s' "$AUDITOR" | tr '[:lower:]-' '[:upper:]_') + { + echo "AUDIT_${key}_START_NS=${AUDIT_START_NS:-$audit_end_ns}" + echo "AUDIT_${key}_END_NS=$audit_end_ns" + echo "AUDIT_${key}_STATUS=$status" + echo "AUDIT_${key}_MODEL=$AUDITOR_MODEL" + } > "audit-$AUDITOR.env" + + - name: Upload seat verdict + if: always() + uses: actions/upload-artifact@v4 + with: + name: audit-${{ env.AUDITOR }}-${{ github.run_id }} + path: | + audit-${{ env.AUDITOR }}.json + audit-${{ env.AUDITOR }}.env + if-no-files-found: error + retention-days: 7 + overwrite: true + + melchior: + needs: context + runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: read + id-token: write # claude-code-action authenticates via OIDC + env: + PR: ${{ inputs.pr }} + AUDITOR: melchior + AUDITOR_MODEL: ${{ vars.MAGI_MODEL_MELCHIOR || 'z-ai/glm-5.3-flash' }} + steps: + - name: Checkout reviewed PR head + uses: actions/checkout@v4 + with: + ref: ${{ needs.context.outputs.head_sha }} + fetch-depth: 0 + + - name: Checkout prompt registry + uses: actions/checkout@v4 + with: + repository: BiggerPockets/.github + ref: ${{ needs.context.outputs.registry_sha }} + token: ${{ secrets.BIGGIEPOCKETS_PAT }} + persist-credentials: false + path: registry + + # The whole of stage 1: the diff, the PR, the ticket, its epic, the discussion, the + # design references, and the manifest saying which of those were available. + - name: Download review handoff + uses: actions/download-artifact@v4 + with: + name: review-context-${{ github.run_id }} + path: . + + - name: Restore review context + run: cat review-context.env >> "$GITHUB_ENV" + + # Resolve again from the same pinned registry ref. Prompt text stays out of job + # outputs (which are size-limited), while the artifact carries all model inputs. + - name: Resolve review prompts + id: resolve + env: + REGISTRY_DIR: registry + run: registry/scripts/resolve-prompts.sh + + # A seat with no model is a lost voice, not a fallback — fail before the panel can + # report a verdict short of a seat it claimed to have. + - name: Confirm seat model + run: | + if [ -z "$AUDITOR_MODEL" ]; then + echo "::error::Auditor '$AUDITOR' has no model: set MAGI_MODEL_$(printf '%s' "$AUDITOR" | tr '[:lower:]-' '[:upper:]_') as a repository variable, or give this job a default." >&2 + exit 1 + fi + echo "Auditor $AUDITOR runs $AUDITOR_MODEL" + + - name: Record audit start time + run: echo "AUDIT_START_NS=$(date +%s%N)" >> "$GITHUB_ENV" + + - name: Audit independently + id: audit + continue-on-error: true + uses: anthropics/claude-code-action@v1 + with: + # The action requires this input for launch validation. ANTHROPIC_AUTH_TOKEN + # below is what makes Claude Code send the OpenRouter key as a bearer token. + anthropic_api_key: ${{ secrets.OPENROUTER_API_KEY }} + # Allow the `claude` bot to trigger this review. Without this, PRs opened + # by the claude bot are rejected with "Workflow initiated by non-human + # actor: claude (type: Bot). Add bot to allowed_bots list." + allowed_bots: 'claude' + claude_args: '--allowedTools "Read,Grep,Glob,Bash,Write" --model ${{ env.AUDITOR_MODEL }}' + prompt: ${{ steps.resolve.outputs.auditor_prompt }} + env: + ANTHROPIC_BASE_URL: "https://openrouter.ai/api" + ANTHROPIC_AUTH_TOKEN: ${{ secrets.OPENROUTER_API_KEY }} + + # Name the verdict for its seat before it leaves this job. The arbitrator reads + # audit-.json, and a seat that produced no usable verdict must be visible as + # such rather than absent: the recorded status is what stops a failed seat from + # being mistaken for a silent approval. + - name: Record seat verdict + if: always() + run: | + audit_end_ns=$(date +%s%N) + status="ok" + [ "${{ steps.audit.outcome }}" != "success" ] && status="error" + + if [ -f verdict.json ] && jq -e '.verdict == "approve" or .verdict == "request_changes"' verdict.json >/dev/null 2>&1; then + mv verdict.json "audit-$AUDITOR.json" + echo "$AUDITOR: $(jq -r '.verdict' "audit-$AUDITOR.json") (confidence $(jq -r '.confidence // "unstated"' "audit-$AUDITOR.json"), $(jq -r '.blocking_findings // [] | length' "audit-$AUDITOR.json") blocking finding(s))" + else + status="error" + rm -f verdict.json + echo "::warning::Auditor $AUDITOR produced no valid verdict.json; the arbitrator will arbitrate without this seat." + fi + + # Shell-safe seat name so the arbitrator can source every seat's file at once. + key=$(printf '%s' "$AUDITOR" | tr '[:lower:]-' '[:upper:]_') + { + echo "AUDIT_${key}_START_NS=${AUDIT_START_NS:-$audit_end_ns}" + echo "AUDIT_${key}_END_NS=$audit_end_ns" + echo "AUDIT_${key}_STATUS=$status" + echo "AUDIT_${key}_MODEL=$AUDITOR_MODEL" + } > "audit-$AUDITOR.env" + + - name: Upload seat verdict + if: always() + uses: actions/upload-artifact@v4 + with: + name: audit-${{ env.AUDITOR }}-${{ github.run_id }} + path: | + audit-${{ env.AUDITOR }}.json + audit-${{ env.AUDITOR }}.env if-no-files-found: error retention-days: 7 overwrite: true # Stage 3. Weighs the three verdicts and issues the one that gets posted. + # + # Runs whenever stage 1 succeeded, even if a seat job failed outright: a seat that + # never started (no model configured, runner lost) would otherwise skip arbitration and + # leave the PR with no review at all. Whether a panel short a seat is still a panel is + # decided by the quorum check below, which says so out loud, rather than by GitHub + # silently skipping this job. arbitrate: - needs: [context, audit] + needs: [context, caspar, balthazar, melchior] + if: ${{ !cancelled() && needs.context.result == 'success' }} runs-on: ubuntu-latest permissions: contents: read diff --git a/.github/workflows/validate-prompts.yml b/.github/workflows/validate-prompts.yml index 9ca3abe..3b21542 100644 --- a/.github/workflows/validate-prompts.yml +++ b/.github/workflows/validate-prompts.yml @@ -173,10 +173,10 @@ jobs: done echo "arbiter prompt names every seat" - # The audit job fans out over this value via fromJSON, so it must be a JSON array - # of strings — a malformed one fails the matrix at run time, after stage 1 has - # already been paid for. - - name: Auditors output is a matrix-ready JSON array + # The arbitrator is told the panel roster through these outputs, and the quorum + # check counts against auditor_count. A malformed roster surfaces mid-review, after + # stage 1 and three audits have already been paid for. + - name: Auditors output is a well-formed panel roster env: PR: '12345' run: | @@ -188,10 +188,60 @@ jobs: count=$(sed -n '/^auditor_count<&2; exit 1; } - # Single line, so it survives being interpolated into the matrix expression. + # Single line, so it survives being interpolated into a workflow expression. test "$(printf '%s' "$auditors" | wc -l | tr -d ' ')" = "0" \ || { echo "auditors output must be compact single-line JSON" >&2; exit 1; } - echo "auditors matrix-ready ($auditors)" + echo "auditors roster well-formed ($auditors)" + + # Each seat is a job of its own, so the panel's shape now lives in two files. These + # are the two ways that can go wrong, and both are invisible on inspection: + # + # - The roster and the jobs disagree. A seat in registry.json with no job never + # runs, and the arbitrator's quorum check reports a panel one seat short of the + # one it was promised. A job with no roster entry runs an audit nobody counts. + # - The seat jobs drift apart. What a disagreement between seats measures is the + # models, so the harness around them has to be identical; three copies of a + # steps block is exactly the kind of thing that drifts one seat at a time. + # Everything seat-specific belongs in the job's `env`, which is why the + # `steps:` blocks can be — and must stay — byte-identical. + - name: Seat jobs match the roster and each other + run: | + wf=.github/workflows/biggiepockets-review.yml + seats=$(jq -r '.auditors[]' prompts/registry.json) + + # A job block runs from ` :` to the next line at that indent. + job_block() { awk -v start=" $1:" ' + index($0, start) == 1 { inside = 1; next } + inside && /^ [^ ]/ { exit } + inside { print }' "$wf"; } + + for seat in $seats; do + grep -qE "^ $seat:$" "$wf" \ + || { echo "registry seat '$seat' has no job in $wf" >&2; exit 1; } + key=$(printf '%s' "$seat" | tr '[:lower:]-' '[:upper:]_') + job_block "$seat" | grep -q "MAGI_MODEL_$key" \ + || { echo "job '$seat' does not read MAGI_MODEL_$key" >&2; exit 1; } + # Byte-identical means identical including the seat name, so nothing is + # normalized away here: the seat name must not appear in the steps at all. + job_block "$seat" | sed -n '/^ steps:$/,$p' > "/tmp/steps-$seat.txt" + test -s "/tmp/steps-$seat.txt" \ + || { echo "job '$seat' has no steps block" >&2; exit 1; } + done + + first=$(printf '%s\n' $seats | head -1) + for seat in $seats; do + diff -u "/tmp/steps-$first.txt" "/tmp/steps-$seat.txt" \ + || { echo "seat job '$seat' has drifted from '$first'" >&2; exit 1; } + done + + # And the other direction: every seat job the workflow defines is on the roster. + declared=$(grep -oE 'MAGI_MODEL_[A-Z0-9_]+' "$wf" | sort -u) + for var in $declared; do + seat=$(printf '%s' "${var#MAGI_MODEL_}" | tr '[:upper:]_' '[:lower:]-') + printf '%s\n' $seats | grep -qx "$seat" \ + || { echo "$wf declares $var but '$seat' is not on the roster" >&2; exit 1; } + done + echo "seat jobs match the roster ($(printf '%s\n' $seats | wc -l | tr -d ' ') seats) and each other" - name: Resolver rejects a broken registry run: | diff --git a/README.md b/README.md index e1cafbe..7ccbdce 100644 --- a/README.md +++ b/README.md @@ -7,9 +7,11 @@ Org-wide GitHub defaults and shared reusable workflows. `.github/workflows/biggiepockets-review.yml` is a **reusable** workflow that reviews a pull request with a **panel** of independent auditors and an arbitrator: -1. **Context** — gathers what the panel judges against: the diff, the PR's JIRA ticket and - its acceptance criteria, and the PR's existing discussion. **No model reviews the code - here.** +1. **Context** — gathers everything the panel judges against and uploads it as one + immutable handoff: the diff, the PR itself, the JIRA ticket and its acceptance criteria, + that ticket's **epic**, the PR's existing discussion, and any **design references** found + in those texts — plus a manifest saying which of them were actually available. **No model + reviews the code here.** 2. **Panel** — three auditors named **caspar**, **balthazar**, and **melchior** each audit the change *independently*, running the *same* auditor prompt, and each writes its own verdict (`approve` / `request_changes`, plus a stated confidence and its blocking @@ -27,8 +29,8 @@ arbitrator's. ```mermaid flowchart LR - ctx["1 · context
diff · ticket + AC · discussion
(no model)"] - subgraph panel["2 · panel — one prompt, three separate jobs"] + ctx["1 · context
diff · PR · ticket + AC · epic
discussion · designs · manifest
(no model)"] + subgraph panel["2 · panel — one prompt, three independent jobs"] direction TB caspar["caspar
openai/gpt-5.6-luna"] balthazar["balthazar
google/gemini-3.7-flash"] @@ -49,12 +51,20 @@ The **BiggiePockets** service account then submits that `approve` / `request_cha on the PR. If the PR has no `BIG-XXXXX` key in its title (or the ticket can't be fetched), the review degrades gracefully to a diff-based review instead of failing. -**What makes the audits independent.** Each seat runs in its own GitHub Actions job, on its -own runner, and can read only the stage-1 handoff — never another seat's output. A seat -writes its verdict to `audit-.json` and uploads it; the arbitration job is the first -and only place in the run where more than one verdict exists together. So when two auditors -disagree, they disagree because they judged the code differently, not because one saw the -other's answer. +**What makes the audits independent.** Each seat is a job of its own — `caspar`, +`balthazar`, `melchior`, each named for the seat and each written out in the workflow rather +than fanned out from a matrix. A seat runs on its own runner, can read only the stage-1 +handoff, and never another seat's output. It writes its verdict to `audit-.json` and +uploads it; the arbitration job is the first and only place in the run where more than one +verdict exists together. So when two auditors disagree, they disagree because they judged the +code differently, not because one saw the other's answer. + +Everything that differs between the three jobs lives in the job's `env` — the seat name and +its model — which leaves their `steps:` blocks byte-identical. That is load-bearing: a split +between seats is only evidence about the models if the harness around them is the same, and +three copies of a block drift one seat at a time. CI diffs the blocks and fails if they stop +matching, and fails too if the set of seat jobs stops matching the roster in +`prompts/registry.json`. The seats run **different models** on the identical prompt (see *Models* below). That is the point: with the prompt held fixed, a split between seats is signal about the change, whereas @@ -69,8 +79,8 @@ but may not introduce a blocking issue no auditor found — it arbitrates rather a fourth reviewer. The posted summary closes with a `Panel:` line recording how the seats split and where the arbitrator overrode them. -**Retries.** Stage 1 uploads the reviewed commit's diff and ticket/discussion context as one -short-lived artifact; every later job downloads that immutable handoff, so all three auditors +**Retries.** Stage 1 uploads the reviewed commit's whole context as one short-lived +artifact; every later job downloads that immutable handoff, so all three auditors judge the exact same commit even if the PR is pushed to mid-review. If a seat is rate-limited, use **Re-run failed jobs** on the workflow run: GitHub re-runs only the failed seats, reusing the context stage that already succeeded. A seat that produces @@ -85,6 +95,39 @@ those two can split 1-1. That is the case where the arbitrator decides alone, so existed. The alternative — demanding all three seats — would let one rate-limited seat fail an entire review. +### What the panel is given + +Stage 1 gathers context and nothing else — it never reads the diff to form a view of it. Each +source becomes one file in the handoff every seat downloads: + +| File | What it holds | +| --- | --- | +| `context-manifest.json` | What was gathered, and which sources were unavailable and why | +| `pr.diff` | The diff of the reviewed commit against its base | +| `pr.json` | Title, body, labels, commit messages, and the touched files with line counts | +| `ticket.json` | The `BIG-XXXXX` ticket from the PR title: summary, description, acceptance criteria, status, type, and its parent's key | +| `epic.json` | That parent — where the wider goal usually lives, when a ticket's own description reads as a fragment | +| `conversations.json` | The PR's existing discussion: comments, inline review threads, prior reviews | +| `designs.json` | Design references found in all of the above: Figma and other design-tool links, screenshots, walkthrough videos, ticket attachments | + +A source that isn't there is recorded as `{"available": false, "reason": …}` rather than +failing the review: a PR with no ticket, no epic and no designs is still reviewable, just with +less to judge intent against. The manifest exists so an auditor can tell *"there is no epic +for this work"* from *"the epic fetch failed"* — both otherwise read as silence, which is how +a review ends up judging intent against nothing and not saying so. The prompts require an +auditor to name context it did not have and what it could not judge without it. + +Design links are **recorded, never followed**. The panel has no Figma credentials, and a code +review has no reason to pull design or member-facing content into a build artifact. What a +recorded link buys an auditor is the knowledge that a design exists for this work — the +difference between "no design was specified" and "a design was specified and I can't see it" — +and the prompts forbid treating a reference it can't open as a defect. + +No handoff file carries an author, assignee or reporter: the panel judges the change, not who +wrote it, and leaving identities out keeps them off the artifacts and out of telemetry. The +PR discussion is the one exception, because attribution is what makes *"a human reviewer +objected and it was never resolved"* legible to the panel. + ### Models Every model an OpenRouter slug with a default pinned in the workflow, overrideable per repo @@ -99,10 +142,10 @@ repo. An unset or empty variable keeps the default. Stage 1 runs no model, so it | Arbitration | `ARBITER_MODEL` | `openai/gpt-5.6-sol` | There is no fallback model. A seat listed in `prompts/registry.json` with no -`MAGI_MODEL_` of its own fails its matrix leg with an explicit error, so adding a seat -is deliberately a two-part change: registry entry plus model. A seat that quietly ran the -same model as another seat would be a panel that had lost a voice while still reporting three -verdicts — worse than a leg that fails and says why. +`MAGI_MODEL_` of its own fails its own job with an explicit error, so adding a seat is +deliberately a three-part change: roster entry, job, model. A seat that quietly ran the same +model as another seat would be a panel that had lost a voice while still reporting three +verdicts — worse than a seat that fails and says why. Changing a seat's model never touches the prompt — the auditor prompt is seat-agnostic by construction, and CI asserts it. @@ -208,9 +251,10 @@ write access. ### Using it If your repo lists a review job as a **required status check**, the job names are `context`, -`audit ()` per seat, and `arbitrate`. Stage 1 was previously named `codex`; a branch -protection rule still requiring `codex` will block merges forever, since no job by that name -runs any more. +one per seat (`caspar`, `balthazar`, `melchior`), and `arbitrate`. Two of those changed: stage +1 was once named `codex`, and the seats were once matrix legs reported as `audit (caspar)` and +friends. A branch protection rule still requiring `codex` or `audit (…)` will block merges +forever, since no job by those names runs any more. Once installed, trigger a review either way: @@ -248,11 +292,13 @@ prompts/ shared blocks it includes — it changes only when that prompt's text changes, not per PR and not per seat. One `auditor_prompt_version` therefore covers all three audit spans, and Datadog LLM Obs can attribute quality to the exact prompt text that ran. -- **Panel seats.** `registry.json` lists the seats under `auditors`. The workflow fans its - audit job out over that list, so adding or removing a seat is a one-line registry change — - no workflow edit. The resolver rejects a panel of fewer than two seats, duplicate seat - names, and seat names that aren't safe as artifact names, file names, and env-var suffixes. - Seat models are set in the workflow, never here (see *Models* above). +- **Panel seats.** `registry.json` lists the seats under `auditors`. That roster is what + tells the arbitrator how many verdicts to expect, so a seat that failed reads as a gap + rather than an absence. Each seat also has a job of its own in the workflow, which means + adding or removing one is a change in both files — CI fails if they disagree. The resolver + rejects a panel of fewer than two seats, duplicate seat names, and seat names that aren't + safe as artifact names, file names, and env-var suffixes. Seat models are set in the + workflow, never here (see *Models* above). - **Prompt Tracking.** Every LLM span carries the prompt that produced it under `meta.input.prompt` — the registry template with its `{{PR}}`-style placeholders intact, plus the values that filled them as `variables`, plus `id`/`name`/`version`. Keeping the diff --git a/prompts/magi-arbitrate.md b/prompts/magi-arbitrate.md index c3de39a..1d3d373 100644 --- a/prompts/magi-arbitrate.md +++ b/prompts/magi-arbitrate.md @@ -10,8 +10,11 @@ request. If one of those files is missing or unparseable, say so explicitly in your summary and arbitrate over the verdicts you do have — never silently treat a missing auditor as an approval. -2. Read the evidence the auditors worked from: pr.diff, ticket.json (its acceptance_criteria - are guidelines, not a hard contract) and conversations.json. +2. Read the evidence the auditors worked from, all of it in the current working directory: + context-manifest.json (what was gathered and what was unavailable), pr.diff, pr.json, + ticket.json (its acceptance_criteria are guidelines, not a hard contract), epic.json, + conversations.json and designs.json (references only — they are not fetched, so a design + you cannot open is never itself a finding). 3. Verify each distinct blocking finding against the code yourself. Use `git log` / `git show` on the branch and grep for callers and tests. Findings often overlap: treat two auditors describing the same defect at the same location as ONE finding raised twice, not diff --git a/prompts/magi-audit.md b/prompts/magi-audit.md index 8431683..b645eb1 100644 --- a/prompts/magi-audit.md +++ b/prompts/magi-audit.md @@ -5,45 +5,66 @@ weighs all three verdicts afterwards. Your job is an honest, self-contained judg not a guess at consensus. No one has reviewed this diff before you: every issue in your verdict is one you found yourself. -1. Read ticket.json. If "available" is true, treat its summary and description as the +Everything you need was gathered for you and sits in the current working directory. + +1. Start with context-manifest.json. It names every file that was gathered and which + sources were unavailable and why. Read it before anything else so you know what you are + working with: a source that isn't there is a gap in the evidence, never a defect in the + change. Do not fetch anything yourself to fill a gap — say in your summary that you + judged without it. +2. Read ticket.json. If "available" is true, treat its summary and description as the intended behavior, and its acceptance_criteria (Atlassian Document Format JSON) as GUIDELINES rather than a hard contract. The AC describe the minimum the ticket set out to achieve, not the ceiling on what counts as correct: don't reject a change merely for not matching them verbatim. Judge whether the change satisfies the ticket's intent, and weigh - git history and the PR discussion (steps 2-3) as more authoritative evidence of what was + git history and the PR discussion (steps 4-5) as more authoritative evidence of what was actually meant. -2. Read conversations.json: the PR's existing discussion (issue_comments, review_comments +3. Read epic.json: the parent the ticket belongs to, where the wider goal usually lives. + Use it to judge whether the change moves that goal forward, and to make sense of a + ticket whose own description reads as a fragment. An epic is context, not a contract — + never block a change for work that plainly belongs to a sibling ticket. +4. Read pr.json: the PR's title and body, labels, commit messages, and the files it + touches with their line counts. The body is usually where the author states the scope + they took on and any follow-up they deliberately left out; weigh that statement against + the ticket rather than ignoring it. +5. Read conversations.json: the PR's existing discussion (issue_comments, review_comments with file/line, and prior reviews). Factor this context into your review: respect decisions already settled in the thread, don't re-raise concerns the author has already addressed or that were explicitly accepted, and weigh unresolved objections raised by human reviewers. Treat the conversation as more reliable than the ticket's literal acceptance criteria: if the thread shows the work was refined or explicitly accepted beyond the AC in a sound way, honor that. -3. Review the diff in pr.diff. Do NOT rely on the hunks alone: use `git log` / +6. Read designs.json: design references (Figma and other design tools, screenshots, + walkthrough videos, ticket attachments) harvested from those texts. They are recorded, + NOT fetched — you cannot open them and must not try. What they tell you is that a design + exists for this work, which matters when the diff changes UI: judge the change against + what the ticket and discussion say about that design, and never treat a reference you + cannot open as a defect. +7. Review the diff in pr.diff. Do NOT rely on the hunks alone: use `git log` / `git show` on the branch to inspect commit history and grep for callers and related tests to judge impact. When the diff adds or changes image assets, check that they are provided at retina quality (e.g. a 2x asset or an `srcset`/`@2x` variant); flag raster images that ship only at 1x where a high-DPI version is expected. {{@prompts/_shared/migration-data-rule.md}} -4. Enforce task completeness and reject incomplete tasks, half-measures, placeholders, or +8. Enforce task completeness and reject incomplete tasks, half-measures, placeholders, or deferred work per these rules as blocking issues: {{@prompts/_shared/completeness-rules.md}} -5. Enforce these BiggerPockets member-privacy rules and treat a genuine violation as a +9. Enforce these BiggerPockets member-privacy rules and treat a genuine violation as a blocking issue: {{@prompts/_shared/privacy-rules.md}} -6. Check for batch/task performance hot spots per these rules — an N+1 or O(n^2)+ +10. Check for batch/task performance hot spots per these rules — an N+1 or O(n^2)+ pattern on a full-table #process/#collection is a genuine finding, not a nitpick; flag it with a suggested fix: {{@prompts/_shared/perf-rules.md}} -7. Check how the diff parses structured values, per these rules: +11. Check how the diff parses structured values, per these rules: {{@prompts/_shared/parsing-rules.md}} -8. Check that in-app navigational links use React Router's Link rather than a raw `` +12. Check that in-app navigational links use React Router's Link rather than a raw `` tag, per these rules: {{@prompts/_shared/navigation-rules.md}} -9. When a ticket is available, judge genuine misses of the ticket's intent or clear scope +13. When a ticket is available, judge genuine misses of the ticket's intent or clear scope creep — but give credit when the author went beyond the literal acceptance criteria in a sound way rather than flagging it as non-compliant. -10. Decide ONE verdict. Be pragmatic: use "request_changes" only when there is at least one +14. Decide ONE verdict. Be pragmatic: use "request_changes" only when there is at least one genuine, blocking issue (such as a bug, regression, privacy violation, half-finished task, placeholder, or deferred work); otherwise "approve". A change that exceeds the AC without breaking the ticket's intent is a reason to approve, not to block. @@ -64,8 +85,9 @@ Write a file named verdict.json in the current working directory with EXACTLY th - "confidence" is how sure you are of your own verdict. Say "low" when the diff's impact turned on code you could not inspect, and reserve "high" for a verdict you could defend line by line. The arbitrator weighs this, so an inflated confidence corrupts the panel. -- In the summary, state your rationale, how the change measures against the ticket, and how - the existing PR discussion informed your decision. +- In the summary, state your rationale, how the change measures against the ticket and its + epic, and how the existing PR discussion informed your decision. Name any context that + was unavailable and what you could not judge without it. Write the file even if you found nothing: an approve with an empty blocking_findings list is a complete verdict. Do not write any other file, and do not post a review or comment — the diff --git a/prompts/registry.json b/prompts/registry.json index f219934..8c0ba60 100644 --- a/prompts/registry.json +++ b/prompts/registry.json @@ -8,20 +8,23 @@ "//": "downloaded by the arbitrator job that runs after all three finish.", "//": "", "//": "Nothing reviews the diff before the panel does. The only work that happens ahead", - "//": "of the auditors is gathering context — the diff, the JIRA ticket and its", - "//": "acceptance criteria, and the PR discussion — so no finding is planted in front of", - "//": "three auditors whose independence is the point.", + "//": "of the auditors is gathering context — the diff, the PR, the JIRA ticket and its", + "//": "acceptance criteria, that ticket's epic, the PR discussion, and any design", + "//": "references found in them — so no finding is planted in front of three auditors", + "//": "whose independence is the point.", "//": "", "//": "The arbitrator then reads all three verdicts, weighs them, and issues the ONE", "//": "verdict that gets posted as the approve / request-changes review. It is the only", "//": "stage whose output reaches the pull request.", "//": "", - "//": "auditors lists the seats. Adding or removing a name changes the panel size; the", - "//": "workflow fans out over this list and the arbitrator is told which seats to expect,", - "//": "so a seat that failed to produce a verdict is a hard error rather than a silently", - "//": "smaller panel. Every seat runs auditor_prompt verbatim — seats differ only in which", - "//": "model they run, which the workflow decides (see MAGI_MODEL_* in", - "//": ".github/workflows/biggiepockets-review.yml), never this file.", + "//": "auditors lists the seats. Each seat is a job of its own in", + "//": ".github/workflows/biggiepockets-review.yml, named for the seat; this list is what", + "//": "tells the arbitrator which seats to expect, so a seat that failed to produce a", + "//": "verdict is a hard error rather than a silently smaller panel. Adding or removing a", + "//": "seat is therefore a two-part change — this list plus that workflow's job and its", + "//": "MAGI_MODEL_ — and CI fails if the two disagree. Every seat runs", + "//": "auditor_prompt verbatim: seats differ only in which model they run, which the", + "//": "workflow decides, never this file.", "//": "", "//": "prompts/.md are versioned templates; shared rule blocks live in", "//": "prompts/_shared/ and are injected via {{@path}} markers. Prompt versions are", diff --git a/scripts/resolve-prompts.sh b/scripts/resolve-prompts.sh index a8abb56..445f928 100755 --- a/scripts/resolve-prompts.sh +++ b/scripts/resolve-prompts.sh @@ -32,8 +32,8 @@ # # Inputs (env): REGISTRY_DIR, PR. # Outputs: auditor_prompt{,_name,_version,_template}; arbiter_prompt{,_name,_version,_template}; -# auditors (JSON array of seat names, which the workflow feeds straight into its fan-out -# matrix); auditor_count. +# auditors (JSON array of seat names — the panel roster the arbitrator is told to expect, +# one job per seat in the workflow); auditor_count. set -euo pipefail @@ -176,7 +176,8 @@ write_output() { # name value — multiline-safe via GITHUB_OUTPUT heredoc; fixe emit_prompt "auditor" "$AUDITOR_NAME" emit_prompt "arbiter" "$ARBITER_NAME" -# Emitted as compact JSON so the workflow can hand it directly to a matrix `include`. +# Emitted as compact JSON, on one line, so it survives interpolation into a workflow +# expression. write_output "auditors" "$(jq -c '.auditors' "$REGISTRY_FILE")" write_output "auditor_count" "${#AUDITORS[@]}" From 15a99fd144729e63bf1d9975ca1496b572b6420d Mon Sep 17 00:00:00 2001 From: Aaron Breckenridge Date: Fri, 28 Aug 2026 12:53:38 -0500 Subject: [PATCH 6/7] Walk the PR stack and tell the panel what it is judging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A PR whose base is not the default branch is stacked on unmerged work, and pr.diff is measured against that base — so the diff holds only this PR's own changes while the branch beneath it carries work that is itself under review. An auditor not told that misreads a stacked PR in both directions: it blames this PR for what an ancestor introduced, and it reports as missing the work an ancestor already did. Both were happening silently, because nothing in the handoff mentioned the stack existed. Stage 1 now walks it — base branch to the PR whose head is that branch, until the default branch — and writes stack.json with each ancestor nearest-first: number, title, body, state, ticket key, commit headlines, touched paths. The walk is capped at ten levels and refuses to revisit a branch, so a cycle left by a retargeted branch cannot hang a review. A base that is neither the default branch nor any PR's head, such as a release branch, is recorded as not a stack with the reason rather than as silence. The prompts rule out both misreadings and point the panel at the questions that are actually this PR's: does it duplicate or contradict an ancestor, does it use an interface the ancestor does not provide, does it only make sense if an ancestor merges first without saying so. Ancestors usually carry a sibling ticket in the same epic, and satisfying that ticket is not this PR's job. The design harvest reads stack.json too — an ancestor's body is a common place for the one Figma link that covers the whole stack. --- .github/workflows/biggiepockets-review.yml | 98 ++++++++++++++++++++-- README.md | 27 ++++-- prompts/magi-arbitrate.md | 9 +- prompts/magi-audit.md | 33 +++++--- 4 files changed, 143 insertions(+), 24 deletions(-) diff --git a/.github/workflows/biggiepockets-review.yml b/.github/workflows/biggiepockets-review.yml index e767660..a6f5657 100644 --- a/.github/workflows/biggiepockets-review.yml +++ b/.github/workflows/biggiepockets-review.yml @@ -6,9 +6,9 @@ name: BiggiePockets Code Review (reusable) # A review runs in three stages: # 1. Context — gathers everything the panel judges against and uploads it as one # immutable handoff: the diff, the PR itself (body, labels, commits, -# touched files), the JIRA ticket and its acceptance criteria, that -# ticket's epic, the PR discussion, and any design references found in -# those texts. Whatever isn't available is recorded as unavailable, in +# touched files), the PRs it is stacked on, the JIRA ticket and its +# acceptance criteria, that ticket's epic, the PR discussion, and any +# design references found in those texts. Whatever isn't available is recorded as unavailable, in # context-manifest.json, so an auditor can tell "there is no epic" from # "the epic fetch failed". No model reviews the code here: nothing # reads the diff ahead of the panel, so no finding is planted in front @@ -178,6 +178,88 @@ jobs: git diff "origin/$BASE_REF...HEAD" > pr.diff echo "PR diff is $(wc -l < pr.diff) lines" + # Walk down the stack. A PR whose base is not the default branch is usually stacked + # on top of another PR, and pr.diff above is computed against that base — so the + # diff is this PR's own changes only, and the branch it sits on contains work that + # is itself under review and can still change. + # + # An auditor not told that reads a stacked PR wrongly in both directions: it blames + # this PR for what the parent introduced, and it reports as missing the work the + # parent already did. Always writes stack.json. + - name: Walk the PR stack + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + default=$(gh repo view "$GITHUB_REPOSITORY" --json defaultBranchRef -q .defaultBranchRef.name) + base=$(jq -r '.base_ref' pr.json) + + note="pr.diff contains ONLY this PR's own changes, measured against its base branch." + if [ "$base" = "$default" ]; then + jq -n --arg default "$default" --arg base "$base" --arg note "$note" \ + '{available: true, stacked: false, reason: "the PR targets the default branch", default_branch: $default, base_ref: $base, depth: 0, ancestors: [], note: $note}' \ + > stack.json + echo "PR targets the default branch ($default); not a stack." + exit 0 + fi + + # Follow base -> the PR whose head is that base, until the default branch. The + # depth cap and the seen-list are guards, not expectations: a retargeted branch + # can leave a cycle behind, and a cycle here would hang the whole review. + ancestors='[]' + b="$base" + seen="" + depth=0 + while [ "$b" != "$default" ] && [ "$depth" -lt 10 ]; do + case " $seen " in *" $b "*) echo "::warning::Stack walk revisited branch $b; stopping."; break;; esac + seen="$seen $b" + + # An open PR for the branch is the one being stacked on; fall back to the most + # recent closed one, which is how a just-merged parent still explains the base. + num=$(gh pr list --repo "$GITHUB_REPOSITORY" --head "$b" --state all --limit 10 \ + --json number,state \ + -q '[.[] | select(.state == "OPEN")] + [.[]] | .[0].number // empty') + if [ -z "$num" ]; then + echo "No pull request has $b as its head; stopping the walk there." + break + fi + + gh pr view "$num" --repo "$GITHUB_REPOSITORY" \ + --json number,title,body,state,url,baseRefName,headRefName,additions,deletions,commits,files \ + > "stack-$num.json" + + ancestors=$(jq -n --argjson a "$ancestors" --slurpfile p "stack-$num.json" \ + '$a + [$p[0] | { + number, title, body, state, url, + base_ref: .baseRefName, + head_ref: .headRefName, + additions, deletions, + ticket_key: ((.title | capture("(?BIG-[0-9]+)").k) // null), + commits: [.commits[] | .messageHeadline], + files: [.files[] | .path] + }]') + + b=$(jq -r '.baseRefName' "stack-$num.json") + depth=$((depth + 1)) + done + + jq -n \ + --arg default "$default" \ + --arg base "$base" \ + --arg note "$note" \ + --argjson ancestors "$ancestors" \ + '{ + available: true, + stacked: (($ancestors | length) > 0), + reason: (if ($ancestors | length) > 0 then "the PR is stacked on unmerged work" + else "the base branch is not the default branch, but no pull request has it as its head" end), + default_branch: $default, + base_ref: $base, + depth: ($ancestors | length), + ancestors: $ancestors, + note: $note + }' > stack.json + echo "Stack: base $base, default $default, $(jq -r '.depth' stack.json) ancestor PR(s) $(jq -r '[.ancestors[].number] | map("#" + tostring) | join(" <- ")' stack.json)" + # Anchor the review on the work's intent. Always writes ticket.json: either the # ticket fields, or {"available": false} so a missing/unfetchable ticket degrades # to a diff-based review instead of failing the review. Also records the parent @@ -314,6 +396,7 @@ jobs: } { urls_from pr.json pr + urls_from stack.json stack urls_from ticket.json ticket urls_from epic.json epic urls_from conversations.json discussion @@ -368,6 +451,7 @@ jobs: --arg base_ref "$BASE_REF" \ --argjson diff_lines "$(wc -l < pr.diff | tr -d " ")" \ --slurpfile pr pr.json \ + --slurpfile stack stack.json \ --slurpfile ticket ticket.json \ --slurpfile epic epic.json \ --slurpfile conversations conversations.json \ @@ -377,6 +461,7 @@ jobs: base_ref: $base_ref, diff: {file: "pr.diff", lines: $diff_lines}, pr: {file: "pr.json", available: true, commits: ($pr[0].commits | length), files: ($pr[0].files | length)}, + stack: {file: "stack.json", stacked: $stack[0].stacked, depth: $stack[0].depth, default_branch: $stack[0].default_branch, ancestors: [$stack[0].ancestors[].number]}, ticket: {file: "ticket.json", available: $ticket[0].available, key: ($ticket[0].key // null), reason: ($ticket[0].reason // null), has_acceptance_criteria: ((($ticket[0].acceptance_criteria // {}) | [.. | objects | select(.type == "text") | .text] | length) > 0)}, epic: {file: "epic.json", available: $epic[0].available, key: ($epic[0].key // null), reason: ($epic[0].reason // null)}, conversations: {file: "conversations.json", issue_comments: ($conversations[0].issue_comments | length), review_comments: ($conversations[0].review_comments | length), reviews: ($conversations[0].reviews | length)}, @@ -407,9 +492,9 @@ jobs: echo "CONTEXT_END_NS=$(date +%s%N)" } > review-context.env - # The raw API responses are deliberately NOT uploaded: they carry fields nobody - # reviews (assignee, reporter, watchers) and the reshaped files above are what the - # prompts are written against. + # The raw API responses (and the per-ancestor stack-.json the walk fetched) are + # deliberately NOT uploaded: they carry fields nobody reviews (assignee, reporter, + # watchers) and the reshaped files above are what the prompts are written against. - name: Upload review handoff uses: actions/upload-artifact@v4 with: @@ -417,6 +502,7 @@ jobs: path: | pr.diff pr.json + stack.json ticket.json epic.json conversations.json diff --git a/README.md b/README.md index 7ccbdce..4a7520d 100644 --- a/README.md +++ b/README.md @@ -8,10 +8,10 @@ Org-wide GitHub defaults and shared reusable workflows. request with a **panel** of independent auditors and an arbitrator: 1. **Context** — gathers everything the panel judges against and uploads it as one - immutable handoff: the diff, the PR itself, the JIRA ticket and its acceptance criteria, - that ticket's **epic**, the PR's existing discussion, and any **design references** found - in those texts — plus a manifest saying which of them were actually available. **No model - reviews the code here.** + immutable handoff: the diff, the PR itself, the **PRs it is stacked on**, the JIRA ticket + and its acceptance criteria, that ticket's **epic**, the PR's existing discussion, and any + **design references** found in those texts — plus a manifest saying which of them were + actually available. **No model reviews the code here.** 2. **Panel** — three auditors named **caspar**, **balthazar**, and **melchior** each audit the change *independently*, running the *same* auditor prompt, and each writes its own verdict (`approve` / `request_changes`, plus a stated confidence and its blocking @@ -29,7 +29,7 @@ arbitrator's. ```mermaid flowchart LR - ctx["1 · context
diff · PR · ticket + AC · epic
discussion · designs · manifest
(no model)"] + ctx["1 · context
diff · PR · stack · ticket + AC · epic
discussion · designs · manifest
(no model)"] subgraph panel["2 · panel — one prompt, three independent jobs"] direction TB caspar["caspar
openai/gpt-5.6-luna"] @@ -105,6 +105,7 @@ source becomes one file in the handoff every seat downloads: | `context-manifest.json` | What was gathered, and which sources were unavailable and why | | `pr.diff` | The diff of the reviewed commit against its base | | `pr.json` | Title, body, labels, commit messages, and the touched files with line counts | +| `stack.json` | The PRs this one is stacked on, nearest first, when its base isn't the default branch | | `ticket.json` | The `BIG-XXXXX` ticket from the PR title: summary, description, acceptance criteria, status, type, and its parent's key | | `epic.json` | That parent — where the wider goal usually lives, when a ticket's own description reads as a fragment | | `conversations.json` | The PR's existing discussion: comments, inline review threads, prior reviews | @@ -117,6 +118,22 @@ for this work"* from *"the epic fetch failed"* — both otherwise read as silenc a review ends up judging intent against nothing and not saying so. The prompts require an auditor to name context it did not have and what it could not judge without it. +**Stacked PRs.** When a PR's base isn't the default branch, stage 1 walks down the stack — +base branch to the PR whose head is that branch, until it reaches the default branch — and +records each ancestor's number, title, body, state, ticket key, commit headlines, and touched +paths. This matters because `pr.diff` is measured against the base, so it holds only this +PR's own changes while the branch beneath it carries work that is itself still under review. +An auditor not told that misreads a stacked PR in both directions: it blames this PR for what +an ancestor introduced, and it reports as missing the work an ancestor already did. The +prompts rule both out, and instead direct the panel at the questions that are this PR's — does +it duplicate or contradict an ancestor, does it use an interface the ancestor doesn't provide, +does it only make sense if an ancestor merges first without saying so. Ancestors often carry a +sibling ticket in the same epic, and satisfying that ticket is not this PR's job. + +The walk is capped at ten levels and refuses to revisit a branch, so a cycle left behind by a +retargeted branch can't hang a review. A base that is neither the default branch nor any PR's +head — a long-lived release branch — is recorded as not a stack, with the reason. + Design links are **recorded, never followed**. The panel has no Figma credentials, and a code review has no reason to pull design or member-facing content into a build artifact. What a recorded link buys an auditor is the knowledge that a design exists for this work — the diff --git a/prompts/magi-arbitrate.md b/prompts/magi-arbitrate.md index 1d3d373..5a64414 100644 --- a/prompts/magi-arbitrate.md +++ b/prompts/magi-arbitrate.md @@ -12,9 +12,12 @@ request. approval. 2. Read the evidence the auditors worked from, all of it in the current working directory: context-manifest.json (what was gathered and what was unavailable), pr.diff, pr.json, - ticket.json (its acceptance_criteria are guidelines, not a hard contract), epic.json, - conversations.json and designs.json (references only — they are not fetched, so a design - you cannot open is never itself a finding). + stack.json, ticket.json (its acceptance_criteria are guidelines, not a hard contract), + epic.json, conversations.json and designs.json (references only — they are not fetched, so + a design you cannot open is never itself a finding). When stack.json says "stacked" is + true, pr.diff is this PR's own changes on top of unmerged ancestor PRs: discard a finding + that belongs to an ancestor's code, and discard "this is missing" when an ancestor's files + or commits already did it. 3. Verify each distinct blocking finding against the code yourself. Use `git log` / `git show` on the branch and grep for callers and tests. Findings often overlap: treat two auditors describing the same defect at the same location as ONE finding raised twice, not diff --git a/prompts/magi-audit.md b/prompts/magi-audit.md index b645eb1..2065cae 100644 --- a/prompts/magi-audit.md +++ b/prompts/magi-audit.md @@ -27,44 +27,57 @@ Everything you need was gathered for you and sits in the current working directo touches with their line counts. The body is usually where the author states the scope they took on and any follow-up they deliberately left out; weigh that statement against the ticket rather than ignoring it. -5. Read conversations.json: the PR's existing discussion (issue_comments, review_comments +5. Read stack.json. If "stacked" is true, this PR sits on top of one or more pull requests + that are themselves unmerged, listed nearest-first in "ancestors". This changes what you + are judging: pr.diff is ONLY this PR's own changes, measured against its base branch, so + the base already contains the ancestors' work. + - Never raise a finding about code an ancestor introduced. It is under review in its own + PR, and the same objection filed twice against two PRs is noise. + - Never report as missing something an ancestor already did. Check its "files" and + commit headlines before concluding a piece of the ticket was skipped. + - Do judge this PR's fit with the stack: work that duplicates or contradicts an ancestor, + an interface used here that the ancestor does not provide, or a change that only makes + sense if an ancestor is merged first and does not say so. + - Judge this PR against ITS ticket. Ancestors often carry a different ticket in the same + epic ("ticket_key"), and satisfying a sibling ticket is not this PR's job. +6. Read conversations.json: the PR's existing discussion (issue_comments, review_comments with file/line, and prior reviews). Factor this context into your review: respect decisions already settled in the thread, don't re-raise concerns the author has already addressed or that were explicitly accepted, and weigh unresolved objections raised by human reviewers. Treat the conversation as more reliable than the ticket's literal acceptance criteria: if the thread shows the work was refined or explicitly accepted beyond the AC in a sound way, honor that. -6. Read designs.json: design references (Figma and other design tools, screenshots, +7. Read designs.json: design references (Figma and other design tools, screenshots, walkthrough videos, ticket attachments) harvested from those texts. They are recorded, NOT fetched — you cannot open them and must not try. What they tell you is that a design exists for this work, which matters when the diff changes UI: judge the change against what the ticket and discussion say about that design, and never treat a reference you cannot open as a defect. -7. Review the diff in pr.diff. Do NOT rely on the hunks alone: use `git log` / +8. Review the diff in pr.diff. Do NOT rely on the hunks alone: use `git log` / `git show` on the branch to inspect commit history and grep for callers and related tests to judge impact. When the diff adds or changes image assets, check that they are provided at retina quality (e.g. a 2x asset or an `srcset`/`@2x` variant); flag raster images that ship only at 1x where a high-DPI version is expected. {{@prompts/_shared/migration-data-rule.md}} -8. Enforce task completeness and reject incomplete tasks, half-measures, placeholders, or +9. Enforce task completeness and reject incomplete tasks, half-measures, placeholders, or deferred work per these rules as blocking issues: {{@prompts/_shared/completeness-rules.md}} -9. Enforce these BiggerPockets member-privacy rules and treat a genuine violation as a +10. Enforce these BiggerPockets member-privacy rules and treat a genuine violation as a blocking issue: {{@prompts/_shared/privacy-rules.md}} -10. Check for batch/task performance hot spots per these rules — an N+1 or O(n^2)+ +11. Check for batch/task performance hot spots per these rules — an N+1 or O(n^2)+ pattern on a full-table #process/#collection is a genuine finding, not a nitpick; flag it with a suggested fix: {{@prompts/_shared/perf-rules.md}} -11. Check how the diff parses structured values, per these rules: +12. Check how the diff parses structured values, per these rules: {{@prompts/_shared/parsing-rules.md}} -12. Check that in-app navigational links use React Router's Link rather than a raw `` +13. Check that in-app navigational links use React Router's Link rather than a raw `` tag, per these rules: {{@prompts/_shared/navigation-rules.md}} -13. When a ticket is available, judge genuine misses of the ticket's intent or clear scope +14. When a ticket is available, judge genuine misses of the ticket's intent or clear scope creep — but give credit when the author went beyond the literal acceptance criteria in a sound way rather than flagging it as non-compliant. -14. Decide ONE verdict. Be pragmatic: use "request_changes" only when there is at least one +15. Decide ONE verdict. Be pragmatic: use "request_changes" only when there is at least one genuine, blocking issue (such as a bug, regression, privacy violation, half-finished task, placeholder, or deferred work); otherwise "approve". A change that exceeds the AC without breaking the ticket's intent is a reason to approve, not to block. From ce725a668e9f49bcef95a0f18c89378c7cef6d69 Mon Sep 17 00:00:00 2001 From: Aaron Breckenridge Date: Fri, 28 Aug 2026 14:39:02 -0500 Subject: [PATCH 7/7] Stop a reviewed PR from supplying its own verdict, and validate what gets posted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A review runs with the reviewed PR's checkout as its working directory — the same directory the handoff downloads into, the same one the model writes verdict.json to, and the same one the audit-*.json globs read. Nothing distinguished a committed file from a model's output. So a PR could approve itself: commit verdict.json with {"verdict":"approve"}, and if the arbitrator's model exits without writing its own (max turns, a give-up, a write to another path — none of which fail the step), the submit step posts that approval as BiggiePockets. The same hole let a committed audit-anything.json inflate PANEL_SEATS_REPORTING, satisfy quorum with zero real verdicts, and inject seat tags into telemetry. Every job that checks out the PR head now clears the reserved names before anything reads them, and validate-prompts.yml asserts it does — this is exactly the invariant that vanishes when a job is added by copying another and trimming it. A PR that legitimately contains one of these paths loses it from the working tree for the length of the review; it is still in pr.diff, which is what gets reviewed. Also from the same review pass: - Arbitration's verdict.json is now validated before the review is posted, the way each seat already validates its own. `jq -r` on a missing key yields the string "null", so an arbitrator that wrote a verdict with no summary would post a review whose entire body was the word null. - A JIRA error body no longer takes down the review. `curl -o` writes the response whatever the status, so a 401 or a proxy's HTML page landed in ticket-raw.json and the design harvest ran jq over HTML, failed under set -e, and failed the context job — contradicting the documented promise that an unfetchable ticket degrades to a diff-based review. - The Datadog run_id includes github.run_attempt. Re-running failed jobs is a documented path, and two reviews of one PR sharing a join key collide in any eval that groups by it. - The stack walk seeds its seen-list with the PR's own head branch, so a base chain returning to it cannot report the panel that this PR is stacked on itself, and it ignores cross-repository PRs, since --head matches on branch name alone and a fork's branch is not our ancestor. - README and the workflow header no longer claim a fallback-model guard that cannot fire, and the last "matrix leg" wording is gone. --- .github/workflows/biggiepockets-review.yml | 124 +++++++++++++++++++-- .github/workflows/validate-prompts.yml | 30 +++++ README.md | 27 +++-- 3 files changed, 159 insertions(+), 22 deletions(-) diff --git a/.github/workflows/biggiepockets-review.yml b/.github/workflows/biggiepockets-review.yml index a6f5657..2f9a19c 100644 --- a/.github/workflows/biggiepockets-review.yml +++ b/.github/workflows/biggiepockets-review.yml @@ -37,10 +37,10 @@ name: BiggiePockets Code Review (reusable) # MAGI_MODEL_BALTHAZAR balthazar (default google/gemini-3.7-flash) # MAGI_MODEL_MELCHIOR melchior (default z-ai/glm-5.3-flash) # ARBITER_MODEL arbitration (default openai/gpt-5.6-sol) -# Every seat carries its own model in its own job. There is no shared fallback on -# purpose: a seat quietly running the same model as another seat is a panel that has lost -# a voice while still reporting three verdicts, which is worse than a seat that fails and -# says so. +# Every seat carries its own model in its own job, and no seat can inherit another's: a +# seat quietly running the same model as another seat is a panel that has lost a voice +# while still reporting three verdicts. A roster entry with no job fails CI; a job whose +# model expression resolves to nothing fails at its `Confirm seat model` step. # The three seats run different models on purpose: the auditor prompt is identical, so # what a disagreement between seats measures is the models, and three runs of one model # would mostly agree trivially and leave the arbitrator nothing to weigh. @@ -103,6 +103,18 @@ jobs: ref: refs/pull/${{ inputs.pr }}/head fetch-depth: 0 + # Same reason as the panel jobs below: this job's working directory is the PR's own + # checkout. Every file this stage publishes is written unconditionally, but the raw + # API responses are read back only when the fetch that produces them ran — so a PR + # that committed ticket-raw.json could otherwise inject attachments into the handoff. + - name: Clear reserved handoff filenames + run: | + rm -rf verdict.json audit-*.json audit-*.env \ + pr.diff pr.json stack.json ticket.json epic.json \ + conversations.json designs.json context-manifest.json review-context.env \ + pr-raw.json ticket-raw.json epic-raw.json urls.json attachments.json \ + stack-*.json + # Pin every later job to the exact commit this context was gathered from. This # matters when a failed audit or arbitration job is retried after the PR has # received another push. @@ -207,7 +219,10 @@ jobs: # can leave a cycle behind, and a cycle here would hang the whole review. ancestors='[]' b="$base" - seen="" + # Seeded with this PR's own head branch. A base chain that comes back around to + # it would otherwise match this very PR by branch name and report the panel that + # it is stacked on itself. + seen=" $(jq -r '.head_ref' pr.json) " depth=0 while [ "$b" != "$default" ] && [ "$depth" -lt 10 ]; do case " $seen " in *" $b "*) echo "::warning::Stack walk revisited branch $b; stopping."; break;; esac @@ -215,9 +230,13 @@ jobs: # An open PR for the branch is the one being stacked on; fall back to the most # recent closed one, which is how a just-merged parent still explains the base. + # Cross-repository PRs are excluded: --head matches on branch name alone, so a + # fork whose branch happens to be named like our base is not our ancestor. num=$(gh pr list --repo "$GITHUB_REPOSITORY" --head "$b" --state all --limit 10 \ - --json number,state \ - -q '[.[] | select(.state == "OPEN")] + [.[]] | .[0].number // empty') + --json number,state,isCrossRepository \ + -q '[.[] | select(.isCrossRepository == false)] + | ([.[] | select(.state == "OPEN")] + .) + | .[0].number // empty') if [ -z "$num" ]; then echo "No pull request has $b as its head; stopping the walk there." break @@ -402,9 +421,14 @@ jobs: urls_from conversations.json discussion } | jq -s '.' > urls.json - if [ -f ticket-raw.json ]; then + # Read the raw ticket only when the fetch actually succeeded, and treat an + # unparseable body as no attachments. `curl -o` writes the response body whatever + # the status, so a 401 or a proxy's HTML error page lands in ticket-raw.json — + # and a jq failure here would fail the context job and take down a review that + # is supposed to degrade to a diff-based one. + if [ "$(jq -r '.available' ticket.json)" = "true" ] && [ -f ticket-raw.json ]; then jq '[.fields.attachment // [] | .[] | {filename, mime_type: .mimeType, size, url: .content, source: "ticket"}]' \ - ticket-raw.json > attachments.json + ticket-raw.json > attachments.json 2>/dev/null || echo '[]' > attachments.json else echo '[]' > attachments.json fi @@ -549,6 +573,22 @@ jobs: ref: ${{ needs.context.outputs.head_sha }} fetch-depth: 0 + # A reviewed PR is untrusted input, and this job's working directory IS the PR's + # checkout — the same directory the handoff downloads into, the same one the model + # writes its verdict to, and the same one the globs downstream read. A PR that + # committed verdict.json, or any audit-*.json, would otherwise hand this stage a + # verdict it wrote itself: nothing further down can tell a committed file from a + # model's output. Clear those names before anything reads them. + # + # A PR that legitimately contains one of these paths loses it from the working tree + # for the length of the review. It is still in pr.diff, which is what gets reviewed. + - name: Clear reserved handoff filenames + run: | + rm -rf verdict.json audit-*.json audit-*.env \ + review-context.env context-manifest.json \ + pr.diff pr.json stack.json ticket.json epic.json \ + conversations.json designs.json + - name: Checkout prompt registry uses: actions/checkout@v4 with: @@ -667,6 +707,22 @@ jobs: ref: ${{ needs.context.outputs.head_sha }} fetch-depth: 0 + # A reviewed PR is untrusted input, and this job's working directory IS the PR's + # checkout — the same directory the handoff downloads into, the same one the model + # writes its verdict to, and the same one the globs downstream read. A PR that + # committed verdict.json, or any audit-*.json, would otherwise hand this stage a + # verdict it wrote itself: nothing further down can tell a committed file from a + # model's output. Clear those names before anything reads them. + # + # A PR that legitimately contains one of these paths loses it from the working tree + # for the length of the review. It is still in pr.diff, which is what gets reviewed. + - name: Clear reserved handoff filenames + run: | + rm -rf verdict.json audit-*.json audit-*.env \ + review-context.env context-manifest.json \ + pr.diff pr.json stack.json ticket.json epic.json \ + conversations.json designs.json + - name: Checkout prompt registry uses: actions/checkout@v4 with: @@ -785,6 +841,22 @@ jobs: ref: ${{ needs.context.outputs.head_sha }} fetch-depth: 0 + # A reviewed PR is untrusted input, and this job's working directory IS the PR's + # checkout — the same directory the handoff downloads into, the same one the model + # writes its verdict to, and the same one the globs downstream read. A PR that + # committed verdict.json, or any audit-*.json, would otherwise hand this stage a + # verdict it wrote itself: nothing further down can tell a committed file from a + # model's output. Clear those names before anything reads them. + # + # A PR that legitimately contains one of these paths loses it from the working tree + # for the length of the review. It is still in pr.diff, which is what gets reviewed. + - name: Clear reserved handoff filenames + run: | + rm -rf verdict.json audit-*.json audit-*.env \ + review-context.env context-manifest.json \ + pr.diff pr.json stack.json ticket.json epic.json \ + conversations.json designs.json + - name: Checkout prompt registry uses: actions/checkout@v4 with: @@ -910,6 +982,22 @@ jobs: ref: ${{ needs.context.outputs.head_sha }} fetch-depth: 0 + # A reviewed PR is untrusted input, and this job's working directory IS the PR's + # checkout — the same directory the handoff downloads into, the same one the model + # writes its verdict to, and the same one the globs downstream read. A PR that + # committed verdict.json, or any audit-*.json, would otherwise hand this stage a + # verdict it wrote itself: nothing further down can tell a committed file from a + # model's output. Clear those names before anything reads them. + # + # A PR that legitimately contains one of these paths loses it from the working tree + # for the length of the review. It is still in pr.diff, which is what gets reviewed. + - name: Clear reserved handoff filenames + run: | + rm -rf verdict.json audit-*.json audit-*.env \ + review-context.env context-manifest.json \ + pr.diff pr.json stack.json ticket.json epic.json \ + conversations.json designs.json + - name: Checkout prompt registry uses: actions/checkout@v4 with: @@ -1040,6 +1128,19 @@ jobs: exit 1 fi + # Validate before posting, the same way each seat validates its own verdict. This + # is the only text that reaches the pull request, and `jq -r` on a missing key + # yields the string "null" — which would post a review whose entire body is the + # word null rather than failing and saying why. + if ! jq -e '(.verdict == "approve" or .verdict == "request_changes") + and (.summary | type == "string") and (.summary | length > 0)' \ + verdict.json >/dev/null 2>&1; then + echo "::error::Arbitration wrote a verdict.json that is not a usable verdict:" >&2 + jq -c '{verdict, summary_type: (.summary | type), summary_length: (.summary | tostring | length)}' verdict.json >&2 2>/dev/null \ + || echo "(unparseable JSON)" >&2 + exit 1 + fi + verdict=$(jq -r '.verdict' verdict.json) summary=$(jq -r '.summary' verdict.json) @@ -1179,7 +1280,10 @@ jobs: arbiter_model_tag="${ARBITER_MODEL:-unspecified}" - run_id="$GITHUB_REPOSITORY-pr$PR-${{ github.run_id }}" + # Includes the attempt: "Re-run failed jobs" is a documented path, and two + # reviews of the same PR sharing a join key would collide in every eval that + # groups by it. + run_id="$GITHUB_REPOSITORY-pr$PR-${{ github.run_id }}-${{ github.run_attempt }}" trace_id=$(openssl rand -hex 16) root_span_id=$(openssl rand -hex 8) diff --git a/.github/workflows/validate-prompts.yml b/.github/workflows/validate-prompts.yml index 3b21542..c719e8b 100644 --- a/.github/workflows/validate-prompts.yml +++ b/.github/workflows/validate-prompts.yml @@ -243,6 +243,36 @@ jobs: done echo "seat jobs match the roster ($(printf '%s\n' $seats | wc -l | tr -d ' ') seats) and each other" + # A reviewed PR is untrusted input, and every job that checks out the PR head has that + # checkout as its working directory — the same directory verdict.json and the + # audit-*.json globs are read from. Without a step that clears those names first, a PR + # can commit a file the review mistakes for a model's verdict and approve itself. This + # is the kind of invariant that disappears when someone adds a job by copying another + # one and trimming it, so it is asserted rather than reviewed. + - name: Every PR checkout clears the reserved handoff names + run: | + wf=.github/workflows/biggiepockets-review.yml + job_block() { awk -v start=" $1:" ' + index($0, start) == 1 { inside = 1; next } + inside && /^ [^ ]/ { exit } + inside { print }' "$wf"; } + + checked=0 + for job in $(grep -oE '^ [a-z][a-z0-9_-]*:' "$wf" | tr -d ' :'); do + block=$(job_block "$job") + printf '%s' "$block" | grep -qE 'name: Checkout (reviewed )?PR head' || continue + checked=$((checked + 1)) + printf '%s' "$block" | grep -q 'name: Clear reserved handoff filenames' \ + || { echo "job '$job' checks out the PR head without clearing reserved handoff names" >&2; exit 1; } + for reserved in verdict.json 'audit-\*.json' pr.diff ticket.json; do + printf '%s' "$block" | grep -q -- "$reserved" \ + || { echo "job '$job' does not clear $reserved" >&2; exit 1; } + done + done + test "$checked" -ge 4 \ + || { echo "expected at least 4 jobs checking out the PR head, found $checked" >&2; exit 1; } + echo "all $checked PR checkouts clear the reserved handoff names" + - name: Resolver rejects a broken registry run: | # Each of these misconfigurations would otherwise surface as a confusing diff --git a/README.md b/README.md index 4a7520d..aaa3da0 100644 --- a/README.md +++ b/README.md @@ -147,7 +147,7 @@ objected and it was never resolved"* legible to the panel. ### Models -Every model an OpenRouter slug with a default pinned in the workflow, overrideable per repo +Every model is an OpenRouter slug with a default pinned in the workflow, overrideable per repo by setting the matching **Actions variable** (Settings → Actions → Variables) in the calling repo. An unset or empty variable keeps the default. Stage 1 runs no model, so it has no entry. @@ -158,11 +158,12 @@ repo. An unset or empty variable keeps the default. Stage 1 runs no model, so it | melchior | `MAGI_MODEL_MELCHIOR` | `z-ai/glm-5.3-flash` | | Arbitration | `ARBITER_MODEL` | `openai/gpt-5.6-sol` | -There is no fallback model. A seat listed in `prompts/registry.json` with no -`MAGI_MODEL_` of its own fails its own job with an explicit error, so adding a seat is -deliberately a three-part change: roster entry, job, model. A seat that quietly ran the same -model as another seat would be a panel that had lost a voice while still reporting three -verdicts — worse than a seat that fails and says why. +There is no fallback model, and no seat can inherit another's. Adding a seat is deliberately +a three-part change — roster entry, job, model — with each part failing on its own terms if +you skip it: a roster entry with no job fails CI, and a job whose model expression resolves to +nothing fails that job at its `Confirm seat model` step. What none of them do is let the seat +run, because a seat quietly sharing another seat's model is a panel that has lost a voice +while still reporting three verdicts. Changing a seat's model never touches the prompt — the auditor prompt is seat-agnostic by construction, and CI asserts it. @@ -356,12 +357,14 @@ prompts/ stored prompt (no version change). Callers tracking `@main` pick the change up on their next run. A caller that pins `uses:` to a tag or SHA needs BOTH that `@ref` and its `registry_ref` input bumped in lockstep — Apply owns that ref-bump explicitly. -- **Seat** — add or remove a name in `auditors`. A new seat also needs a - `MAGI_MODEL_` default in the workflow (or that repository variable set); without - one its matrix leg fails rather than silently doubling up on another seat's model. +- **Seat** — add or remove a name in `auditors`, **and** the matching job in + `.github/workflows/biggiepockets-review.yml` with its `MAGI_MODEL_`. CI fails if the + roster and the seat jobs disagree, or if the new job's `steps:` block isn't byte-identical + to the others. A `validate-prompts.yml` workflow guards the registry: it fails a PR if a template has dangling includes, `registry.json` references a missing prompt, the auditor prompt names a -seat, the arbitrator prompt doesn't name every seat, the seat list isn't a matrix-ready JSON -array of at least two safe unique names, a broken registry resolves instead of erroring, the -resolver isn't deterministic for a fixed PR, or a shared-rule edit doesn't bump versions. +seat, the arbitrator prompt doesn't name every seat, the roster isn't a well-formed list of +at least two safe unique names, the seat jobs don't match the roster and each other, a broken +registry resolves instead of erroring, the resolver isn't deterministic for a fixed PR, or a +shared-rule edit doesn't bump versions.