From 3c541cc7b407ee79fd4a726c37fc6d060d060fe5 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Wed, 29 Jul 2026 12:35:36 -0400 Subject: [PATCH] fix: never report an unread CI signal as missing CI The first live run reported the reviewed PR's CI as "missing" while it was actually green, and the model cited that as a reason to withhold approval. Reading check-runs needs the App's checks:read permission, which the App did not have, so the call 403'd and the fallback turned an authorization failure into the factual claim "this PR has no CI". That is the exact failure this runner exists to prevent, applied to a governance signal instead of a gate. Distinguish the two: an unreadable signal is now recorded as "unknown", the evidence report says unknown is not evidence of absent or failing CI, and the skill tells the reviewer that a signal the runner could not read is not a finding. Request checks:read at token mint so the signal is readable at all. Also correct the App verification snippet in the README: listing an installation's selected repositories needs the installation token, so the documented /user/installations call does not work for an org admin. --- .github/workflows/review-pr.yml | 1 + README.md | 27 +++++++++++++++++---------- scripts/generate-evidence.sh | 2 +- scripts/resolve-pr-head.sh | 30 ++++++++++++++++++++++-------- skills/pr-review/SKILL.md | 5 +++++ 5 files changed, 46 insertions(+), 19 deletions(-) diff --git a/.github/workflows/review-pr.yml b/.github/workflows/review-pr.yml index 5a05441..5b1ba18 100644 --- a/.github/workflows/review-pr.yml +++ b/.github/workflows/review-pr.yml @@ -115,6 +115,7 @@ jobs: # installation is later granted broader permissions. permission-contents: read permission-pull-requests: read + permission-checks: read permission-metadata: read - name: Resolve frontend PR head diff --git a/README.md b/README.md index bc0c41d..8cbb025 100644 --- a/README.md +++ b/README.md @@ -61,24 +61,31 @@ missing or rejected. ## GitHub App -The App must have **exactly** these permissions and nothing else, installed on -**only** `shoc-frontend-new` and `shoc-backend`: +The App (`shoc-review-runner`) must have **exactly** these permissions and +nothing else, installed on **only** `shoc-frontend-new` and `shoc-backend`: - Repository permissions: Contents **Read-only**, Pull requests **Read-only**, - Metadata **Read-only**. + Checks **Read-only**, Metadata **Read-only**. + +Checks read is what lets the runner report the reviewed PR's own CI status. +Without it the runner records that signal as `unknown` rather than guessing. Verify after installing (and re-verify when the App changes): ```sh -APP_INSTALLS=$(gh api /orgs/Sea-Haven-Industries/installations --jq \ - '.installations[] | select(.app_slug=="")') -echo "$APP_INSTALLS" | jq '{permissions, repository_selection}' -# permissions must be exactly {contents: "read", pull_requests: "read", metadata: "read"} -gh api "/user/installations/$(echo "$APP_INSTALLS" | jq -r .id)/repositories" \ - --jq '.repositories[].full_name' -# must list exactly the two product repos +gh api /orgs/Sea-Haven-Industries/installations \ + --jq '.installations[] | select(.app_slug=="shoc-review-runner") + | {permissions, repository_selection}' +# permissions must be exactly contents/pull_requests/checks/metadata = "read", +# and repository_selection must be "selected" ``` +Listing the selected repositories requires the installation token itself +(`GET /installation/repositories`), which only exists inside a workflow run. +In practice the token mint is the check: `actions/create-github-app-token` +requests `shoc-frontend-new,shoc-backend` explicitly and fails the run if the +App is not installed on both. + ## Security architecture Reviewing a PR means building it, and building it means executing code the PR diff --git a/scripts/generate-evidence.sh b/scripts/generate-evidence.sh index dd5f0a6..88a93fc 100755 --- a/scripts/generate-evidence.sh +++ b/scripts/generate-evidence.sh @@ -76,7 +76,7 @@ cat >"$EVIDENCE_FILE" </dev/null)" \ - || checks_json='{"total":0,"runs":[]}' -ci_status="$(jq -r ' - if .total == 0 then "missing" - elif ([.runs[] | select(.status != "completed")] | length) > 0 then "pending" - elif ([.runs[] | select(.conclusion != "success" and .conclusion != "neutral" and .conclusion != "skipped")] | length) > 0 then "red" - else "green" end' <<<"$checks_json")" +# CI status on the exact head: green / red / pending / missing / unknown. +# +# A failed API call must NEVER be reported as "missing": that would state as a +# fact ("this PR has no CI") what is actually an unread signal, which is the +# exact failure mode this runner exists to prevent. Reading check-runs needs the +# App's `checks: read` permission; without it the call 403s and the honest +# answer is "unknown". +checks_err="$ARTIFACTS_DIR/$side-checks-error.txt" +if checks_json="$(gh api "repos/$repo/commits/$head_sha/check-runs" \ + --jq '{total: .total_count, runs: [.check_runs[] | {name, status, conclusion}]}' 2>"$checks_err")"; then + ci_status="$(jq -r ' + if .total == 0 then "missing" + elif ([.runs[] | select(.status != "completed")] | length) > 0 then "pending" + elif ([.runs[] | select(.conclusion != "success" and .conclusion != "neutral" and .conclusion != "skipped")] | length) > 0 then "red" + else "green" end' <<<"$checks_json")" + rm -f "$checks_err" +else + reason="$(tr -d '\000-\037' <"$checks_err" | cut -c1-160)" + log "WARNING: could not read check-runs for $repo@$short_sha: $reason" + ci_status="unknown (check-runs unreadable; the review App likely lacks 'checks: read')" + checks_json='{"total":0,"runs":[],"unreadable":true}' +fi jq -n \ --arg side "$side" --arg repo "$repo" --argjson pr "$pr" \ diff --git a/skills/pr-review/SKILL.md b/skills/pr-review/SKILL.md index aa901ec..197dd2c 100644 --- a/skills/pr-review/SKILL.md +++ b/skills/pr-review/SKILL.md @@ -52,6 +52,11 @@ Sort every problem before writing the review: * **Environment limitation** — a gate the runner could not execute (see evidence "Runtime Limitations"). State it; never convert it into a pass or a code defect. +A signal the runner could not read is not a finding. If a governance signal is +recorded as `unknown`, say nothing about it or note only that it was unavailable +to the runner — never report it as missing, failing, or a reason to withhold +approval. Only `missing`, `red`, or `pending` are statements about the PR. + ## Verdict discipline * `APPROVE` only when every required gate in the evidence is PASS or