mirror of
https://github.com/Sea-Haven-Industries/shoc-pr-review-runner.git
synced 2026-09-30 08:13:13 +00:00
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.
This commit is contained in:
parent
c68a71bf8a
commit
3c541cc7b4
5 changed files with 46 additions and 19 deletions
1
.github/workflows/review-pr.yml
vendored
1
.github/workflows/review-pr.yml
vendored
|
|
@ -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
|
||||
|
|
|
|||
27
README.md
27
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=="<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
|
||||
|
|
|
|||
|
|
@ -76,7 +76,7 @@ cat >"$EVIDENCE_FILE" <<EOF
|
|||
- Backend PR CI status: $(pr_field backend '.ci_status')
|
||||
- Frontend PR mergeable: $(pr_field frontend '.mergeable // "unknown"')
|
||||
- Backend PR mergeable: $(pr_field backend '.mergeable // "unknown"')
|
||||
- CI statuses come from the PR head's check-runs; red/pending/missing is a governance signal, not silently omitted.
|
||||
- CI statuses come from the PR head's check-runs. "missing" means the API returned zero checks; "unknown" means the runner could not read them and is NOT evidence that CI is absent or failing. Do not treat "unknown" as a governance problem with the PR.
|
||||
|
||||
## Stack Status
|
||||
- Frontend parent: NOT_RUN (stacked-PR resolution is Phase 4)
|
||||
|
|
|
|||
|
|
@ -27,14 +27,28 @@ head_repo="$(jq -r '.head.repo.full_name // empty' <<<"$pr_json")"
|
|||
|
||||
short_sha="${head_sha:0:7}"
|
||||
|
||||
# CI status on the exact head: green / red / pending / missing (never omitted).
|
||||
checks_json="$(gh api "repos/$repo/commits/$head_sha/check-runs" --jq '{total: .total_count, runs: [.check_runs[] | {name, status, conclusion}]}' 2>/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" \
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue