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