diff --git a/scripts/dependabot-digest/classify.sh b/scripts/dependabot-digest/classify.sh index 739ba5e..c909073 100755 --- a/scripts/dependabot-digest/classify.sh +++ b/scripts/dependabot-digest/classify.sh @@ -94,6 +94,15 @@ output="$(jq -c --argjson hold "${hold_labels}" --argjson neutral_ok "${neutral_ and (.name as $n | any($neutral_ok[]; . as $p | $n | startswith($p)) | not); . as $pr + # A check with no name is one the token could not read, not one that passed. + # GitHub serves statusCheckRollup with HTTP 200 and the right totalCount, + # then nulls every CheckRun a fine-grained token lacks Checks: read for — + # which cannot be granted to one at all + # (github.com/orgs/community/discussions/129512). Counting those as absent + # turns seven red builds into "nothing failing" and puts the PR in + # ready-to-merge. collect.sh refuses such a response outright; this is the + # second line of defence, in the component that decides what is safe. + | ([.checks[] | select((.name // null) == null)]) as $unreadable | ([.checks[] | select(.required)]) as $req | ([$req[] | select(failed)]) as $req_failed | ([$req[] | select(pending)]) as $req_pending @@ -128,8 +137,12 @@ output="$(jq -c --argjson hold "${hold_labels}" --argjson neutral_ok "${neutral_ [$neutral_blocking[] | "NEUTRAL: " + .name] + (if $changes_requested then ["review: CHANGES_REQUESTED"] else [] end)) + | .unreadableChecks = ($unreadable | length) | .bucket = ( if ($held_by | length) > 0 or .isDraft then "held" + # Unreadable checks outrank every other signal: nothing below can be + # trusted when the check data is known to be incomplete. + elif ($unreadable | length) > 0 then "checks-unreadable" elif ($req_failed | length) > 0 then "needs-work" elif .mergeStateStatus == "DIRTY" or .mergeable == "CONFLICTING" then "conflicted" elif ($req_pending | length) > 0 then "waiting-on-ci" diff --git a/scripts/dependabot-digest/collect.sh b/scripts/dependabot-digest/collect.sh index fd2b002..ce6fe23 100755 --- a/scripts/dependabot-digest/collect.sh +++ b/scripts/dependabot-digest/collect.sh @@ -50,9 +50,15 @@ GRAPHQL verify_private_visibility() { local probe="$1" [[ -z "${probe}" ]] && return 0 - if ! gh api "repos/${probe}" --jq '.name' >/dev/null 2>&1; then - echo "collect.sh: ${owner}: cannot read private probe repo ${probe};" \ - "token lacks private-repo access, so results would be silently incomplete" >&2 + # Probe `commits`, not `repos/`. Repository metadata answers under the + # Metadata permission alone, so a metadata probe passes for a token that + # cannot read a single commit — and the survey reads every PR's checks + # through `pullRequest.commits`. Measured 2026-09-11: this probe passed on + # nightowlstudiollc while every PR detail read returned FORBIDDEN, which is + # exactly the silent incompleteness it exists to prevent. + if ! gh api "repos/${probe}/commits?per_page=1" --jq '.[0].sha' >/dev/null 2>&1; then + echo "collect.sh: ${owner}: cannot read commits on private probe repo ${probe};" \ + "token lacks the access this survey needs, so results would be silently incomplete" >&2 return 1 fi return 0 @@ -156,14 +162,27 @@ while IFS=$'\t' read -r nwo number; do echo "collect.sh: ${nwo}#${number}: could not read PR detail (exit ${detail_rc})" >&2 # GraphQL returns errors in the body with HTTP 200, so both streams matter. if [[ -s "${detail_err}" ]]; then - detail_err_text="$(tr '\n' ' ' <"${detail_err}")" + # gh concatenates one copy of the message per GraphQL error, so a single + # refused permission prints the same sentence eleven times on one line. + # The graphql line below already reports the count, so truncate here + # rather than repeat it. + detail_err_text="$(tr '\n' ' ' <"${detail_err}" | cut -c1-200)" echo "collect.sh: stderr: ${detail_err_text}" >&2 fi # Parenthesize each alternative: `+` binds tighter than `//`, so # `.type // "?" + ": " + .message` parses as `.type // ("?: " + .message)` # and yields a bare "FORBIDDEN" with the message dropped — exactly the # detail this block exists to print. Verified against a sample error body. - gql_errors="$(jq -r '.errors // [] | map((.type // "?") + ": " + (.message // "?")) | join("; ")' <<<"${detail}" 2>/dev/null)" + # Collapse repeats: one refused permission yields one error per context, + # and eleven identical lines bury the one fact that matters. Group by + # type+message, report the first path and how many followed it. + gql_errors="$(jq -r '.errors // [] + | group_by((.type // "?") + "" + (.message // "?")) + | map((.[0].type // "?") + " at " + + (((.[0].path // []) | map(tostring) | join("."))) + + (if length > 1 then " (and \(length - 1) more)" else "" end) + + ": " + (.[0].message // "?")) + | join("; ")' <<<"${detail}" 2>/dev/null)" if [[ -n "${gql_errors}" && "${gql_errors}" != "null" ]]; then echo "collect.sh: graphql: ${gql_errors}" >&2 fi @@ -171,6 +190,26 @@ while IFS=$'\t' read -r nwo number; do exit 1 fi rm -f "${detail_err}" + # A context that comes back as an empty object is an access failure wearing + # the shape of a result. GitHub returns the rollup with HTTP 200 and the + # right totalCount, then nulls every CheckRun the token may not read — + # measured 2026-09-11, where 11 of 12 contexts were null while only the + # StatusContext survived. Mapped naively that becomes "no failing checks and + # no required checks", which classifies a PR with seven red builds as + # ready-to-merge and offers a merge-lock line for it. Refuse instead: an + # unreadable check is not an absent one. + # A readable context always carries a name (CheckRun) or a context + # (StatusContext). A nulled one carries neither, and arrives as `{}`. + nulled="$(jq '[.data.repository.pullRequest.commits.nodes[0].commit.statusCheckRollup.contexts.nodes[]? + | select((.name // .context) == null)] | length' <<<"${detail}" 2>/dev/null)" + if [[ -n "${nulled}" && "${nulled}" != "null" && "${nulled}" -gt 0 ]]; then + echo "collect.sh: ${nwo}#${number}: ${nulled} check(s) returned null —" \ + "the token can see that checks exist but not what they say." \ + "A fine-grained token cannot grant Checks: read" \ + "(github.com/orgs/community/discussions/129512), so this survey would" \ + "under-report failures rather than fail. Refusing to continue." >&2 + exit 1 + fi jq -c --arg nwo "${nwo}" --argjson base_red "${base_red}" ' .data.repository.pullRequest as $pr | ([$pr.commits.nodes[0].commit.statusCheckRollup.contexts.nodes[]? diff --git a/scripts/dependabot-digest/render.sh b/scripts/dependabot-digest/render.sh index 595e88e..e8f5df1 100755 --- a/scripts/dependabot-digest/render.sh +++ b/scripts/dependabot-digest/render.sh @@ -52,7 +52,10 @@ else # The one fact that explains this row. Naming the specific check beats a # generic status: "build" sends someone to a file, "UNSTABLE" does not. def reason: - if .bucket == "needs-work" then "required check failed: " + (.blockingFailures | join(", ")) + if .bucket == "checks-unreadable" then + "CHECK DATA INCOMPLETE: " + (.unreadableChecks | tostring) + + " check(s) unreadable — treat nothing here as green" + elif .bucket == "needs-work" then "required check failed: " + (.blockingFailures | join(", ")) elif .bucket == "hook-blocked" then (.hookBlockers | join("; ")) elif .bucket == "advisory-red" then (if (.ownFailures | length) > 0 @@ -74,6 +77,8 @@ else echo } + render_bucket checks-unreadable "Check results could not be read" \ + "The token could not read these PRs' check results, so their status is unknown — not green. GitHub returns the rollup with the right count and nulls the checks it will not show, which reads as \"nothing failing\" unless caught. A fine-grained token cannot be granted Checks: read at all (github.com/orgs/community/discussions/129512). Do not merge on the strength of this section." render_bucket ready-to-merge "Ready for a merge-lock" \ "Every required check passed and the pre-merge hook has no mechanical objection. The hook still runs its AI review on merge, which can block on content — this list means nothing stands in the way yet, not that the merge will succeed." render_bucket update-branch "Needs a branch update" \ diff --git a/scripts/dependabot-digest/tests/test-classify.sh b/scripts/dependabot-digest/tests/test-classify.sh index 2da6771..bb22c5c 100755 --- a/scripts/dependabot-digest/tests/test-classify.sh +++ b/scripts/dependabot-digest/tests/test-classify.sh @@ -165,6 +165,28 @@ else _fail "classify.sh failed on the real fixture" fi + +# A check whose fields came back null is an access failure, not a passing +# check. GitHub returns statusCheckRollup with HTTP 200 and the correct +# totalCount, then nulls every CheckRun a fine-grained token may not read — +# Checks: read cannot be granted to one at all +# (github.com/orgs/community/discussions/129512). Measured 2026-09-11: 11 of 12 +# contexts null, and the one survivor was a Netlify StatusContext. +# +# collect.sh refuses that response outright. This asserts the consequence if it +# ever stops doing so: nulled checks must never read as "nothing is failing". +nulled='{"repo":"o/r","number":9,"title":"chore: bump foo from 1.0.0 to 2.0.0","createdAt":"2026-09-01T00:00:00Z","updatedAt":"2026-09-01T00:00:00Z","isDraft":false,"mergeable":"MERGEABLE","mergeStateStatus":"UNSTABLE","reviewDecision":null,"autoMerge":false,"labels":[],"checks":[{"name":null,"conclusion":null,"status":null,"required":null}],"baseRed":[]}' +nulled_bucket="$(bash "${CLASSIFY}" <<<"${nulled}" | jq -r '.bucket')" +if [[ -z "${nulled_bucket}" ]]; then + _fail "the nulled-checks assertion produced no bucket — it would pass vacuously" +fi +if [[ "${nulled_bucket}" == "ready-to-merge" ]]; then + _fail "a PR whose checks are all null classified as ready-to-merge — a token that cannot read checks would recommend merging failing PRs" +else + _pass "nulled checks do not classify as ready-to-merge (got ${nulled_bucket})" +fi + + if [[ "${fail}" -eq 0 ]]; then echo "test-classify: all assertions passed" else diff --git a/scripts/dependabot-digest/tests/test-render.sh b/scripts/dependabot-digest/tests/test-render.sh index 327d25f..c8c7c82 100755 --- a/scripts/dependabot-digest/tests/test-render.sh +++ b/scripts/dependabot-digest/tests/test-render.sh @@ -38,17 +38,32 @@ done # dropped — the failure is invisible until the moment you need the message. # Extract the error formatter from collect.sh and run it against a real error # body rather than eyeballing the source. -gql_fmt="$(grep -o "jq -r '\.errors[^']*'" "${DIR}/collect.sh" | head -1 | sed "s/^jq -r '//; s/'$//")" +# The formatter spans several lines, so extract from `jq -r '.errors` to the +# closing quote rather than matching a single line. A line-based grep silently +# found nothing once the program was reformatted, which failed the assertion +# for the wrong reason. +gql_fmt="$(sed -n "/jq -r '\.errors/,/'[[:space:]]*<</dev/null)" if [[ "${formatted}" == *"FORBIDDEN"* && "${formatted}" == *"not accessible"* ]]; then _pass "the GraphQL error formatter keeps both the type and the message" else _fail "the error formatter dropped part of the error: '${formatted}'" fi + if [[ "${formatted}" == *"commits"* ]]; then + _pass "the GraphQL error formatter keeps the field path" + else + _fail "the error formatter dropped the path, which names the failing field: '${formatted}'" + fi fi body="${WORK}/body.md" diff --git a/scripts/dependabot-digest/tests/test-run-digest.sh b/scripts/dependabot-digest/tests/test-run-digest.sh index 840f25d..c14a6f4 100755 --- a/scripts/dependabot-digest/tests/test-run-digest.sh +++ b/scripts/dependabot-digest/tests/test-run-digest.sh @@ -88,7 +88,7 @@ PATH="${BIN}:${PATH}" DIGEST_OWNERS="one" DIGEST_TOKEN_ONE="t" \ DIGEST_PROBE_ONE="one/private" \ bash "${DIR}/run-digest.sh" --dry-run >/dev/null 2>"${WORK}/probe.err" probe_rc=$? -if [[ "${probe_rc}" -ne 0 ]] && grep -q "cannot read private probe repo" "${WORK}/probe.err"; then +if [[ "${probe_rc}" -ne 0 ]] && grep -q "cannot read commits on private probe repo" "${WORK}/probe.err"; then _pass "an unreadable private probe fails the run" else _fail "an unreadable private probe did not fail the run (exit ${probe_rc})"