From 65c4309f667688e90b8883a813b9dea841108d66 Mon Sep 17 00:00:00 2001 From: cestercian Date: Thu, 17 Sep 2026 22:07:08 +0000 Subject: [PATCH] fix: settle reactions and clean verdicts from one authoritative run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings, one root: every publication and every reaction was decided from one run's local view. Five review rounds added a condition per site; this is the mechanism those findings wanted. Identity: GET /user has no authenticated user for an installation token, so the fallback hard-coded github-actions[bot] and a custom App's 👀 never matched the delete filter. The 👀 POST already returns the author; that login is carried in $RUNNER_TEMP/cr-actor, and settle also asks the installation for its app slug. github-actions[bot] is last, not first. Authority: a run may publish a clean verdict or settle reactions only when it still describes the current head (unknown = no) and no newer sibling of the same workflow exists. The head comparison used to live only in the thumb step, after the clean review — including the issue-comment fallback, which has no commit association — had already been posted. Both call sites now consult scripts/authority.mjs. The 👍 is still placed and never withdrawn (#40). A stale run simply does not get addThumb. Tests: 32 new, covering identity order, superseded heads, newer siblings, User-token decline, the CLI, and that action.yml no longer guesses the Actions bot. --- action.yml | 240 ++++++++++++++--------- scripts/authority.mjs | 221 +++++++++++++++++++++ scripts/authority.test.mjs | 379 +++++++++++++++++++++++++++++++++++++ 3 files changed, 755 insertions(+), 85 deletions(-) create mode 100644 scripts/authority.mjs create mode 100644 scripts/authority.test.mjs diff --git a/action.yml b/action.yml index 1b15546..5fb6337 100644 --- a/action.yml +++ b/action.yml @@ -248,7 +248,7 @@ runs: "$RUNNER_TEMP/pr.diff" "$RUNNER_TEMP/diff-guard.json" "$RUNNER_TEMP/prev-summary.md" \ "$RUNNER_TEMP/wallclock-timeout" "$RUNNER_TEMP/judge-unavailable" \ "$RUNNER_TEMP/cr-usage.jsonl" \ - "$RUNNER_TEMP/cr-clean" \ + "$RUNNER_TEMP/cr-clean" "$RUNNER_TEMP/cr-actor" "$RUNNER_TEMP/cr-plan.json" \ "$RUNNER_TEMP/result.l1.json" "$RUNNER_TEMP/result.l2.json" \ "$RUNNER_TEMP/result-extra.l1.json" "$RUNNER_TEMP/result-extra.l2.json" \ "$RUNNER_TEMP/result.l2.log" "$RUNNER_TEMP/result-extra.l2.log" @@ -335,28 +335,33 @@ runs: exit 0 fi - # JUST REACT. This used to record the id the POST returned so the clear - # step could delete exactly it — first unconditionally, then only on a - # 201 — and each version needed a JSON parse that node is not yet - # available for at this point in the job. The last one was a greedy - # expression that picked the nested author id out of a compact response, - # so the DELETE would have targeted the wrong resource. + # JUST REACT — and carry who we reacted as. The id used to be recorded + # so the clear could delete exactly it; every version needed a parse + # that node is not yet available for, and the last one picked the + # nested author id out of a compact response. The id is still not + # needed: the settle step declines to manage reactions at all when + # github-token is a user, so there is nothing here to tell "ours" + # from the token owner's. # - # The id existed to tell OUR reaction from the token owner's. That - # question is settled once, in the settle step, by declining to manage - # reactions at all when github-token is a user rather than an app — so - # there is nothing here to record, and nothing to parse. + # The LOGIN is. GET /user has no authenticated user for an + # installation token, and guessing github-actions[bot] left every + # custom App's 👀 in place forever. The POST already returns the + # author — `gh api --jq` reads it without node — and settle + # (scripts/authority.mjs) uses that observed identity first. # # Two concurrent runs as the same app share one reaction, so the first to - # finish clears it while the second is still working. Accepted: a missing - # 👀 for a few minutes is cosmetic, and every mechanism that avoided it - # cost more correctness elsewhere than it bought. + # finish clears it while the second is still working. Accepted as a + # missing-👀 cosmetic only when the first run is still the authority; + # a stale run finishing first now stands down (same planner). react() { - if gh api -X POST "$1" -f content=eyes >/dev/null 2>&1; then - echo "reacted 👀 -> $1" - else - echo "warning: failed to react at $1 (non-fatal)" - fi + login=$(gh api -X POST "$1" -f content=eyes --jq .user.login 2>/dev/null) || login="" + case "$login" in + ""|null|undefined) echo "warning: failed to react at $1 (non-fatal)" ;; + *) + printf '%s\n' "$login" > "$RUNNER_TEMP/cr-actor" + echo "reacted 👀 -> $1 as $login" + ;; + esac } # Always react on the PR body (issue reactions endpoint — PRs are issues). if [ -n "${PR_NUMBER:-}" ]; then @@ -1352,15 +1357,59 @@ runs: # to declare a run clean put "No findings" and a 👍 on a pull request # whose own summary listed the findings three lines above. RESULT_FOUND: ${{ runner.temp }}/result-reported.json + AUTHORITY: ${{ github.action_path }}/scripts/authority.mjs + RUN_ID: ${{ github.run_id }} + WORKFLOW: ${{ github.workflow }} with: github-token: ${{ inputs.github-token }} script: | const fs = require('fs'); + const { execFileSync } = require('child_process'); const brand = process.env.BRAND; const num = Number(process.env.PR_NUMBER); const sha = process.env.HEAD_SHA; const repo = { owner: context.repo.owner, repo: context.repo.repo }; + // ONE PLANNER for clean publication and (later) reaction settlement. + // A head comparison living only in the always() settle step cannot + // prevent or retract a clean verdict this step already posted, and + // the issue-comment fallback has no commit association at all. + const planClean = async () => { + let current = ''; + try { + const { data: live } = await github.rest.pulls.get({ ...repo, pull_number: num }); + current = live.head && live.head.sha || ''; + } catch (e) { + console.log('could not re-read the PR head (non-fatal):', e.message); + } + let siblings = '[]'; + try { + const { data } = await github.rest.actions.listWorkflowRunsForRepo({ + ...repo, head_sha: sha, per_page: 50, + }); + siblings = JSON.stringify((data.workflow_runs || []).map((r) => ({ id: r.id, name: r.name }))); + } catch (e) { + // actions:read is not in the example workflow. Head match still + // answers finding 3; sibling comparison is best-effort. + console.log('could not list sibling runs (non-fatal):', e.message); + } + try { + return JSON.parse(execFileSync('node', [ + process.env.AUTHORITY, '--settle', + '--reviewed', sha || '', + '--current', current, + '--run-id', process.env.RUN_ID || '', + '--workflow', process.env.WORKFLOW || '', + '--siblings', siblings, + '--clean', + ], { encoding: 'utf8' })); + } catch (e) { + // Fail closed for a clean verdict: "could not tell" is not a yes. + console.log('settlement planner failed (non-fatal):', e.message); + return { publishClean: false, reason: 'settlement planner failed' }; + } + }; + let result = { comments: [] }; try { result = JSON.parse(fs.readFileSync(process.env.RESULT, 'utf8')); @@ -1426,6 +1475,17 @@ runs: // Still one timeline entry per push, which was the original worry. // That is what the 👍 is for: the entry states the result once, and // the reaction is the at-a-glance version that updates in place. + // + // ONLY WHEN THIS RUN IS STILL THE AUTHORITY. The planner is the + // same function the settle step uses: a superseded head, or a + // newer sibling on this head, must not publish "✅ No findings" + // — the check that used to live only in reaction settlement + // could neither prevent nor retract this message. + const allowed = await planClean(); + if (!allowed.publishClean) { + console.log(`not posting a clean verdict: ${allowed.reason}`); + return; + } const cleanBody = `## 🐳 ${brand}\n\n` + `✅ **No findings** — nothing to flag in this PR. Great work!`; @@ -1723,47 +1783,71 @@ runs: HEAD_SHA: ${{ steps.pr.outputs.head_sha }} EVENT_NAME: ${{ github.event_name }} COMMENT_ID: ${{ github.event.comment.id }} + AUTHORITY: ${{ github.action_path }}/scripts/authority.mjs + RUN_ID: ${{ github.run_id }} + WORKFLOW: ${{ github.workflow }} run: | - # ONE GATE FOR BOTH REACTIONS: is this token an app? - # - # GitHub cannot be asked "which reaction did I create" after the fact, so - # the only handle is the author — and when github-token is a maintainer's - # PAT, their manual 👀 or 👍 and ours have the same author. Three rounds - # of review went into narrowing that predicate with recorded ids and - # status codes; every version needed a parse, and the parses were where - # the bugs were. Declining at the boundary removes the question instead - # of answering it more precisely each time. + # FACTS, then ONE PLAN. Identity, "are we still the authority", and + # which reactions to write used to be a gate at each call site — a + # guessed github-actions[bot] login, a head comparison only the thumb + # saw, a delete the clean post had already raced. scripts/authority.mjs + # is that decision; this step only gathers what the planner cannot see + # and does what it says. # - # The review body and the summary say everything the reactions would. - # ONLY AN EXPLICIT "User" DECLINES. `gh api user` needs a user-scoped - # token, and github-token defaults to ${{ github.token }} — an - # installation token, for which that endpoint returns no user. The old - # `|| echo "Bot"` fallback was meant to cover that and did not: the call - # can exit 0 with no `.type`, so ME_TYPE came back empty, `!= "Bot"` held, - # and the reaction was skipped for EVERY consumer on the default token — - # the case this exists to serve. Measured on a real run: "belongs to a - # user, not an app" while the reaction author was github-actions[bot]. - # - # Inverted to fail towards acting: a PAT identifies itself as type User, - # and anything else — 403, empty, Bot — reacts. + # ONLY AN EXPLICIT "User" DECLINES, and the planner says so: `gh api + # user` needs a user-scoped token, and github-token defaults to + # ${{ github.token }} — an installation token, for which that + # endpoint returns no user. A PAT identifies itself as type User; + # 403, empty, Bot all react. ME_TYPE=$(gh api user --jq .type 2>/dev/null) || ME_TYPE="" - if [ "$ME_TYPE" = "User" ]; then - echo "github-token belongs to a user, not an app; leaving reactions alone" + ME_LOGIN=$(gh api user --jq .login 2>/dev/null) || ME_LOGIN="" + CREATED_BY="" + if [ -f "$RUNNER_TEMP/cr-actor" ]; then + CREATED_BY=$(tr -d '\r\n' < "$RUNNER_TEMP/cr-actor") + fi + APP_SLUG=$(gh api "repos/$REPO/installation" --jq .app_slug 2>/dev/null) || APP_SLUG="" + CURRENT_SHA="" + if [ -n "${PR_NUMBER:-}" ]; then + CURRENT_SHA=$(gh api "repos/$REPO/pulls/$PR_NUMBER" --jq .head.sha 2>/dev/null) || CURRENT_SHA="" + fi + SIBLINGS='[]' + if [ -n "${HEAD_SHA:-}" ]; then + SIBLINGS=$(gh api "repos/$REPO/actions/runs?head_sha=${HEAD_SHA}&per_page=50" \ + --jq '[.workflow_runs[] | {id:.id, name:.name}]' 2>/dev/null) || SIBLINGS='[]' + fi + CLEAN_FLAG="" + [ -f "$RUNNER_TEMP/cr-clean" ] && CLEAN_FLAG="--clean" + PLAN_FILE="$RUNNER_TEMP/cr-plan.json" + if ! node "$AUTHORITY" --settle \ + --token-type "$ME_TYPE" \ + --user-login "$ME_LOGIN" \ + --created-by "$CREATED_BY" \ + --app-slug "$APP_SLUG" \ + --reviewed "$HEAD_SHA" \ + --current "$CURRENT_SHA" \ + --run-id "$RUN_ID" \ + --workflow "$WORKFLOW" \ + --siblings "$SIBLINGS" \ + $CLEAN_FLAG > "$PLAN_FILE"; then + echo "warning: settlement planner failed (non-fatal); leaving reactions alone" exit 0 fi - # SAME TRAP AS THE TYPE PROBE, and it survived the fix to that one: with an - # installation token `gh api user` can exit 0 and print nothing, so - # `|| echo` never fires and ME came back EMPTY. The delete filter then read - # `select(.user.login=="")`, matched nothing, and the 👀 stayed on the PR - # for ever while the step reported success. Observed on a clean run that - # placed both 👀 and 👍 and cleared neither. - # - # Test the VALUE, not the exit status. - ME=$(gh api user --jq .login 2>/dev/null) || ME="" - [ -n "$ME" ] || ME="github-actions[bot]" + if [ ! -s "$PLAN_FILE" ]; then + echo "warning: settlement planner produced no plan (non-fatal); leaving reactions alone" + exit 0 + fi + node -e 'const p=JSON.parse(require("fs").readFileSync(process.argv[1],"utf8")); console.log(p.reason||"")' "$PLAN_FILE" + + field() { node -e 'const p=JSON.parse(require("fs").readFileSync(process.env.PLAN_FILE,"utf8")); const v=p[process.argv[1]]; if (typeof v==="boolean") process.exit(v?0:1); process.stdout.write(String(v??""))' "$1"; } + export PLAN_FILE + ME=$(field actor) # ------------------------------------------------------------------ # 👀 — it said "I am looking at this", and the run has stopped looking. + # Only the authoritative run clears: a stale finish must not take the + # acknowledgement off a newer run that is still working, and must not + # match the wrong login (the planner resolved custom-app[bot] from + # the create-response or the installation slug). clear_eyes() { ids=$(gh api "$1" --paginate \ --jq '.[] | select(.content=="eyes") | select(.user.login=="'"$ME"'") | .id' \ @@ -1773,24 +1857,22 @@ runs: && echo "cleared 👀 -> $1/$id" || echo "warning: could not clear 👀 (non-fatal)" done } - if [ -n "${PR_NUMBER:-}" ]; then - clear_eyes "repos/$REPO/issues/$PR_NUMBER/reactions" - fi - if [ "$EVENT_NAME" = "issue_comment" ] && [ -n "${COMMENT_ID:-}" ]; then - clear_eyes "repos/$REPO/issues/comments/$COMMENT_ID/reactions" + if field clearEyes; then + if [ -n "${PR_NUMBER:-}" ]; then + clear_eyes "repos/$REPO/issues/$PR_NUMBER/reactions" + fi + if [ "$EVENT_NAME" = "issue_comment" ] && [ -n "${COMMENT_ID:-}" ]; then + clear_eyes "repos/$REPO/issues/comments/$COMMENT_ID/reactions" + fi + else + echo "not clearing 👀" fi # ------------------------------------------------------------------ - # THE 👍 IS PLACED AND NEVER WITHDRAWN — a product decision, and it - # deletes more code than it adds rules. - # - # Withdrawing was the source of the concurrency problem this step kept - # growing conditions for: with no serialisation between invocations, an - # older findings run finishing after a newer clean one removed a thumb - # that correctly described the current head, and the justification for - # leaving the delete unguarded — "the current run re-adds it" — only held - # when the current run finished last, which nothing guarantees. Remove - # the withdrawal and the whole class goes with it. + # THE 👍 IS PLACED AND NEVER WITHDRAWN — a product decision. The + # planner decides whether THIS run may place it. A stale run on a + # superseded head, or an older sibling finishing after a newer one, + # gets addThumb=false; there is no delete for it to reach. # # What it costs, stated plainly: a head that was clean keeps its 👍 after # a later push breaks something. The review comment on that later push @@ -1798,24 +1880,12 @@ runs: # of this PR reviewed clean" rather than "the current one does". The # hosted reviewer has made the same trade since it shipped, so the two # paths now agree about what the mark means. - # - # ONLY FOR THE HEAD THIS RUN REVIEWED, and that guard matters MORE now - # rather than less: a wrong add is permanent. An unconfirmed head counts - # as moved — "could not tell" is not evidence that it did not. - if [ ! -f "$RUNNER_TEMP/cr-clean" ]; then - echo "not a clean run; leaving 👍 alone" - exit 0 - fi - CURRENT_SHA=$(gh api "repos/$REPO/pulls/$PR_NUMBER" --jq .head.sha 2>/dev/null || echo "") - if [ "$CURRENT_SHA" != "$HEAD_SHA" ]; then - echo "cannot confirm this run still describes the PR head (${HEAD_SHA} vs ${CURRENT_SHA:-unknown}); leaving 👍 alone" - exit 0 + if field addThumb; then + gh api -X POST "repos/$REPO/issues/$PR_NUMBER/reactions" -f content=+1 >/dev/null 2>&1 \ + && echo "👍 -> PR #$PR_NUMBER" || echo "warning: could not add 👍 (non-fatal)" + else + echo "not adding 👍" fi - # POSTED UNCONDITIONALLY. Creating a reaction that already exists is a - # no-op at the API, so there is nothing to look up first — which also - # means the 👍 no longer depends on resolving our own login, and the - # custom-App identity gap is confined to clearing 👀. - gh api -X POST "repos/$REPO/issues/$PR_NUMBER/reactions" -f content=+1 >/dev/null 2>&1 && echo "👍 -> PR #$PR_NUMBER" || echo "warning: could not add 👍 (non-fatal)" - name: Clean up engine output if: always() @@ -1836,7 +1906,7 @@ runs: "$RUNNER_TEMP/pr.diff" "$RUNNER_TEMP/diff-guard.json" "$RUNNER_TEMP/prev-summary.md" \ "$RUNNER_TEMP/wallclock-timeout" "$RUNNER_TEMP/judge-unavailable" \ "$RUNNER_TEMP/cr-usage.jsonl" \ - "$RUNNER_TEMP/cr-clean" \ + "$RUNNER_TEMP/cr-clean" "$RUNNER_TEMP/cr-actor" "$RUNNER_TEMP/cr-plan.json" \ "$RUNNER_TEMP/result.l1.json" "$RUNNER_TEMP/result.l2.json" \ "$RUNNER_TEMP/result-extra.l1.json" "$RUNNER_TEMP/result-extra.l2.json" \ "$RUNNER_TEMP/result.l2.log" "$RUNNER_TEMP/result-extra.l2.log" diff --git a/scripts/authority.mjs b/scripts/authority.mjs new file mode 100644 index 0000000..2841482 --- /dev/null +++ b/scripts/authority.mjs @@ -0,0 +1,221 @@ +#!/usr/bin/env node +// Authoritative-run settlement for the OrcaCode Review action. +// +// One decision, not a guard per call site. The reaction logic used to live in +// bash inside action.yml, which is why five review rounds found what no test +// did: every publication and every reaction was decided from one run's local +// view, and each fix added a condition (an identity match, a recorded id, a +// status code, a head comparison) at the site that had just failed. +// +// This module is the mechanism those three findings wanted. The driver +// gathers facts — token type, the login a reaction-create already returned, +// /user, the installation's app slug, the head this run reviewed, the head +// the PR has now, this run's id, sibling runs — and `settle()` answers what +// that invocation may do. Both the clean-verdict post and the always() +// reaction step consult the same function. +// +// node authority.mjs --settle [--token-type User|Bot] [--user-login …] +// [--created-by …] [--app-slug …] --reviewed --current +// [--run-id ] [--workflow ] [--siblings ] [--clean] +// +// Prints one JSON object and ALWAYS exits 0 on --settle — a settlement +// glitch must not fail the review: +// +// { actor, authoritative, manageReactions, publishClean, clearEyes, +// addThumb, reason } +// +// Identity: GET /user has no authenticated user for an installation token, +// so the old fallback guessed github-actions[bot]. A custom App creates +// reactions as custom-app[bot] and the delete filter never matched. Prefer +// the login the create already returned; then /user; then {app_slug}[bot] +// from the installation; then github-actions[bot] for the default Actions +// token. +// +// Authority: this run is authoritative only when it still describes the +// current head (an unknown current head is "no" — "could not tell" is not +// evidence that it did not move) AND no sibling run of the same workflow +// is newer. Only the authoritative run publishes a clean verdict or +// settles reactions. A stale run finishing after a newer one therefore +// cannot withdraw a current thumb, leave a leftover 👀, or post +// "✅ No findings" for a superseded head. + +import { resolve } from "node:path"; +import { fileURLToPath } from "node:url"; + +export const GITHUB_ACTIONS_BOT = "github-actions[bot]"; + +// Empty, whitespace, and the literal strings jq prints for a missing field +// are all "no value". Installation tokens make `gh api user --jq .login` +// exit 0 and print nothing — or `null` — and the last time that was treated +// as a login the delete filter matched nobody. +function live(v) { + const s = String(v ?? "").trim(); + if (!s || s === "null" || s === "undefined") return ""; + return s; +} + +export function resolveActorLogin({ userLogin, createdByLogin, appSlug } = {}) { + const created = live(createdByLogin); + if (created) return created; + const user = live(userLogin); + if (user) return user; + const slug = live(appSlug); + if (slug) return slug.endsWith("[bot]") ? slug : `${slug}[bot]`; + return GITHUB_ACTIONS_BOT; +} + +export function tokenIsUser(tokenType) { + return live(tokenType) === "User"; +} + +// Sibling runs already scoped by the driver (same workflow / same PR head), +// or carrying a `name` we can filter with --workflow. A CI run with a +// higher id on a different workflow must not steal authority. +export function relevantSiblings(siblingRuns, { workflow } = {}) { + const runs = Array.isArray(siblingRuns) ? siblingRuns : []; + const wf = live(workflow); + return runs.filter((r) => { + if (!r || r.id == null || r.id === "") return false; + if (!wf || !r.name) return true; + return r.name === wf; + }); +} + +export function notAuthoritativeReason({ + reviewedSha, + currentSha, + thisRunId, + siblingRuns, + workflow, +} = {}) { + const reviewed = live(reviewedSha); + const current = live(currentSha); + if (!reviewed || !current) { + return `cannot confirm this run still describes the PR head (${reviewed || "unknown"} vs ${current || "unknown"})`; + } + if (reviewed !== current) { + return `cannot confirm this run still describes the PR head (${reviewed} vs ${current})`; + } + const self = Number(thisRunId); + const newer = relevantSiblings(siblingRuns, { workflow }).find((r) => { + const id = Number(r.id); + return Number.isFinite(id) && Number.isFinite(self) && id > self; + }); + if (newer) { + return `a newer run (${newer.id}) is authoritative; this run is ${live(thisRunId) || "unknown"}`; + } + return ""; +} + +export function isAuthoritative(input = {}) { + return notAuthoritativeReason(input) === ""; +} + +export function settle({ + tokenType, + userLogin, + createdByLogin, + appSlug, + reviewedSha, + currentSha, + thisRunId, + siblingRuns, + workflow, + clean, +} = {}) { + const facts = { reviewedSha, currentSha, thisRunId, siblingRuns, workflow }; + const authoritative = isAuthoritative(facts); + const staleReason = notAuthoritativeReason(facts); + const actor = resolveActorLogin({ userLogin, createdByLogin, appSlug }); + // A PAT-backed run still publishes a review. The user-token decline is + // only about reactions: GitHub cannot tell ours from the token owner's. + if (tokenIsUser(tokenType)) { + return { + actor: "", + authoritative, + manageReactions: false, + publishClean: authoritative, + clearEyes: false, + addThumb: false, + reason: authoritative + ? "github-token belongs to a user, not an app; leaving reactions alone" + : staleReason, + }; + } + if (!authoritative) { + return { + actor, + authoritative: false, + manageReactions: true, + publishClean: false, + clearEyes: false, + addThumb: false, + reason: staleReason, + }; + } + return { + actor, + authoritative: true, + manageReactions: true, + publishClean: true, + clearEyes: true, + addThumb: !!clean, + reason: "this run is authoritative", + }; +} + +function parseArgs(argv) { + const opts = { siblings: [] }; + for (let i = 0; i < argv.length; i += 1) { + const a = argv[i]; + if (a === "--settle") opts.settle = true; + else if (a === "--clean") opts.clean = true; + else if (a === "--token-type") opts.tokenType = argv[++i]; + else if (a === "--user-login") opts.userLogin = argv[++i]; + else if (a === "--created-by") opts.createdByLogin = argv[++i]; + else if (a === "--app-slug") opts.appSlug = argv[++i]; + else if (a === "--reviewed") opts.reviewedSha = argv[++i]; + else if (a === "--current") opts.currentSha = argv[++i]; + else if (a === "--run-id") opts.thisRunId = argv[++i]; + else if (a === "--workflow") opts.workflow = argv[++i]; + else if (a === "--siblings") { + const raw = argv[++i] ?? ""; + try { + const parsed = JSON.parse(raw); + opts.siblings = Array.isArray(parsed) ? parsed : []; + } catch { + opts.siblings = []; + } + } + } + return opts; +} + +function main(argv = process.argv.slice(2)) { + const opts = parseArgs(argv); + if (!opts.settle) { + console.error( + "usage: node authority.mjs --settle [--token-type User|Bot] [--user-login L] " + + "[--created-by L] [--app-slug S] --reviewed SHA --current SHA " + + "[--run-id N] [--workflow NAME] [--siblings JSON] [--clean]", + ); + process.exit(2); + } + const plan = settle({ + tokenType: opts.tokenType, + userLogin: opts.userLogin, + createdByLogin: opts.createdByLogin, + appSlug: opts.appSlug, + reviewedSha: opts.reviewedSha, + currentSha: opts.currentSha, + thisRunId: opts.thisRunId, + siblingRuns: opts.siblings, + workflow: opts.workflow, + clean: opts.clean, + }); + process.stdout.write(`${JSON.stringify(plan)}\n`); +} + +const invokedDirectly = + Boolean(process.argv[1]) && resolve(process.argv[1]) === fileURLToPath(import.meta.url); +if (invokedDirectly) main(); diff --git a/scripts/authority.test.mjs b/scripts/authority.test.mjs new file mode 100644 index 0000000..af55f77 --- /dev/null +++ b/scripts/authority.test.mjs @@ -0,0 +1,379 @@ +// Contract tests for authority.mjs — the single settlement decision behind +// reactions and the clean-verdict post. +// +// Three findings shared one root: nothing in the composite action established +// which invocation was authoritative, so every publication and every reaction +// was decided from one run's local view. The mechanism lives here so it can +// be exercised without adding another condition at each call site. +// +// Pins: +// 1. Custom App identity is resolved (create-response, /user, installation +// slug), not guessed as github-actions[bot]. +// 2. Latest-run-authoritative: a stale sibling cannot settle, even on the +// same head. +// 3. A superseded head cannot publish a clean verdict — unknown current +// head counts as moved. +// +// The driver (action.yml) is pinned to consult this script rather than +// re-implement the fallback, so the two cannot drift apart silently again. + +import { execFileSync, spawnSync } from "node:child_process"; +import { readFileSync } from "node:fs"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { describe, test } from "node:test"; +import assert from "node:assert/strict"; +import { + GITHUB_ACTIONS_BOT, + resolveActorLogin, + tokenIsUser, + isAuthoritative, + notAuthoritativeReason, + relevantSiblings, + settle, +} from "./authority.mjs"; + +const SCRIPTS = dirname(fileURLToPath(import.meta.url)); +const AUTHORITY = join(SCRIPTS, "authority.mjs"); +const ACTION = join(SCRIPTS, "..", "action.yml"); + +function cli(args) { + return JSON.parse(execFileSync("node", [AUTHORITY, "--settle", ...args], { encoding: "utf8" })); +} + +describe("resolveActorLogin", () => { + test("the login a reaction-create returned wins — that is who we must delete", () => { + assert.equal( + resolveActorLogin({ + createdByLogin: "custom-app[bot]", + userLogin: "someone", + appSlug: "other-app", + }), + "custom-app[bot]", + ); + }); + + test("/user login is used when nothing was carried from create", () => { + assert.equal(resolveActorLogin({ userLogin: "my-bot[bot]" }), "my-bot[bot]"); + }); + + test("installation app_slug becomes {slug}[bot]", () => { + assert.equal(resolveActorLogin({ appSlug: "orcacode-review" }), "orcacode-review[bot]"); + }); + + test("an app_slug that already carries [bot] is not doubled", () => { + assert.equal(resolveActorLogin({ appSlug: "orcacode-review[bot]" }), "orcacode-review[bot]"); + }); + + test("empty /user (installation token) does not beat a carried identity", () => { + assert.equal( + resolveActorLogin({ userLogin: "", createdByLogin: "custom-app[bot]" }), + "custom-app[bot]", + ); + }); + + test("jq's literal null is not a login — that was the #43 trap one layer up", () => { + assert.equal( + resolveActorLogin({ userLogin: "null", createdByLogin: "null", appSlug: "null" }), + GITHUB_ACTIONS_BOT, + ); + }); + + test("only when every source is empty do we fall back to github-actions[bot]", () => { + assert.equal(resolveActorLogin({}), GITHUB_ACTIONS_BOT); + assert.equal(resolveActorLogin({ userLogin: "", createdByLogin: " ", appSlug: undefined }), GITHUB_ACTIONS_BOT); + }); +}); + +describe("tokenIsUser", () => { + test("only an explicit User declines — empty/Bot/403-shaped values react", () => { + assert.equal(tokenIsUser("User"), true); + assert.equal(tokenIsUser("Bot"), false); + assert.equal(tokenIsUser(""), false); + assert.equal(tokenIsUser(undefined), false); + assert.equal(tokenIsUser("null"), false); + }); +}); + +describe("isAuthoritative / latest-run", () => { + const head = "abc123"; + + test("unknown current head is not evidence this run still describes it", () => { + assert.equal(isAuthoritative({ reviewedSha: head, currentSha: "" }), false); + assert.equal(isAuthoritative({ reviewedSha: head, currentSha: "null" }), false); + assert.match(notAuthoritativeReason({ reviewedSha: head, currentSha: "" }), /unknown/); + }); + + test("unknown reviewed head cannot publish", () => { + assert.equal(isAuthoritative({ reviewedSha: "", currentSha: head }), false); + }); + + test("a superseded head is not authoritative", () => { + assert.equal(isAuthoritative({ reviewedSha: head, currentSha: "def456" }), false); + assert.match( + notAuthoritativeReason({ reviewedSha: head, currentSha: "def456" }), + /abc123 vs def456/, + ); + }); + + test("matching heads with no sibling evidence -> authoritative", () => { + assert.equal(isAuthoritative({ reviewedSha: head, currentSha: head, thisRunId: "10" }), true); + assert.equal(isAuthoritative({ reviewedSha: head, currentSha: head, siblingRuns: [] }), true); + }); + + test("an older sibling on the same head does not steal authority", () => { + assert.equal( + isAuthoritative({ + reviewedSha: head, + currentSha: head, + thisRunId: "20", + siblingRuns: [{ id: 10 }, { id: 20 }], + }), + true, + ); + }); + + test("a newer sibling on the same head is the authority — this run stands down", () => { + assert.equal( + isAuthoritative({ + reviewedSha: head, + currentSha: head, + thisRunId: "10", + siblingRuns: [{ id: 10 }, { id: 11 }], + }), + false, + ); + assert.match( + notAuthoritativeReason({ + reviewedSha: head, + currentSha: head, + thisRunId: "10", + siblingRuns: [{ id: 11 }], + }), + /newer run \(11\)/, + ); + }); + + test("a sibling from another workflow is ignored when --workflow is set", () => { + assert.equal( + isAuthoritative({ + reviewedSha: head, + currentSha: head, + thisRunId: "10", + workflow: "OrcaCode Review", + siblingRuns: [ + { id: 99, name: "CI" }, + { id: 10, name: "OrcaCode Review" }, + ], + }), + true, + ); + }); + + test("same-workflow newer sibling still wins after the name filter", () => { + assert.equal( + isAuthoritative({ + reviewedSha: head, + currentSha: head, + thisRunId: "10", + workflow: "OrcaCode Review", + siblingRuns: [ + { id: 12, name: "OrcaCode Review" }, + { id: 10, name: "OrcaCode Review" }, + ], + }), + false, + ); + }); + + test("relevantSiblings drops nameless-id-less rows and keeps pre-scoped {id} rows", () => { + assert.deepEqual(relevantSiblings([{ id: 1 }, { name: "x" }, null], { workflow: "x" }), [{ id: 1 }]); + }); +}); + +describe("settle — one plan for publication and reactions", () => { + const current = { + reviewedSha: "abc", + currentSha: "abc", + thisRunId: "5", + createdByLogin: "custom-app[bot]", + }; + + test("authoritative clean run: clear 👀, add 👍, allowed to publish clean", () => { + const plan = settle({ ...current, clean: true }); + assert.equal(plan.actor, "custom-app[bot]"); + assert.equal(plan.authoritative, true); + assert.equal(plan.manageReactions, true); + assert.equal(plan.publishClean, true); + assert.equal(plan.clearEyes, true); + assert.equal(plan.addThumb, true); + assert.equal(plan.reason, "this run is authoritative"); + }); + + test("authoritative findings run: clear 👀, do not add 👍, still may publish clean if it had one", () => { + const plan = settle({ ...current, clean: false }); + assert.equal(plan.clearEyes, true); + assert.equal(plan.addThumb, false); + assert.equal(plan.publishClean, true); + }); + + test("stale run on a superseded head: no clean verdict, no reaction writes", () => { + const plan = settle({ + ...current, + currentSha: "newer", + clean: true, + }); + assert.equal(plan.authoritative, false); + assert.equal(plan.publishClean, false); + assert.equal(plan.clearEyes, false); + assert.equal(plan.addThumb, false); + assert.match(plan.reason, /abc vs newer/); + // Identity is still resolved so a log can name who we would have acted as. + assert.equal(plan.actor, "custom-app[bot]"); + }); + + test("stale run finishing after a newer sibling does not add or clear", () => { + const plan = settle({ + ...current, + thisRunId: "5", + siblingRuns: [{ id: 9 }], + clean: true, + }); + assert.equal(plan.authoritative, false); + assert.equal(plan.publishClean, false); + assert.equal(plan.clearEyes, false); + assert.equal(plan.addThumb, false); + assert.match(plan.reason, /newer run \(9\)/); + }); + + test("a User token never manages reactions, but an authoritative one may still publish", () => { + const plan = settle({ ...current, tokenType: "User", clean: true }); + assert.equal(plan.manageReactions, false); + assert.equal(plan.clearEyes, false); + assert.equal(plan.addThumb, false); + assert.equal(plan.publishClean, true); + assert.equal(plan.actor, ""); + assert.match(plan.reason, /belongs to a user/); + }); + + test("a User token on a superseded head publishes nothing", () => { + const plan = settle({ ...current, tokenType: "User", currentSha: "moved", clean: true }); + assert.equal(plan.publishClean, false); + assert.equal(plan.manageReactions, false); + }); + + test("installation-token identity (no /user) uses the create-response login", () => { + const plan = settle({ + ...current, + tokenType: "", + userLogin: "", + createdByLogin: "acme-review[bot]", + clean: true, + }); + assert.equal(plan.actor, "acme-review[bot]"); + assert.equal(plan.clearEyes, true); + }); + + test("installation-token identity with only an app slug, no create-response", () => { + const plan = settle({ + reviewedSha: "abc", + currentSha: "abc", + userLogin: "", + createdByLogin: "", + appSlug: "acme-review", + clean: true, + }); + assert.equal(plan.actor, "acme-review[bot]"); + }); +}); + +describe("CLI", () => { + test("--settle prints the plan and exits 0 (a planner glitch must not fail the job)", () => { + const plan = cli([ + "--reviewed", + "abc", + "--current", + "abc", + "--run-id", + "3", + "--created-by", + "custom-app[bot]", + "--clean", + ]); + assert.equal(plan.addThumb, true); + assert.equal(plan.actor, "custom-app[bot]"); + }); + + test("garbage --siblings is no sibling evidence, not a crash", () => { + const plan = cli(["--reviewed", "abc", "--current", "abc", "--siblings", "not-json"]); + assert.equal(plan.authoritative, true); + }); + + test("missing --settle is usage (exit 2)", () => { + const r = spawnSync("node", [AUTHORITY], { encoding: "utf8" }); + assert.equal(r.status, 2); + assert.match(r.stderr, /usage:/); + }); + + test("a newer sibling via --siblings and --workflow stands this run down", () => { + const plan = cli([ + "--reviewed", + "abc", + "--current", + "abc", + "--run-id", + "1", + "--workflow", + "OrcaCode Review", + "--siblings", + JSON.stringify([ + { id: 2, name: "OrcaCode Review" }, + { id: 99, name: "CI" }, + ]), + "--clean", + ]); + assert.equal(plan.authoritative, false); + assert.equal(plan.addThumb, false); + assert.equal(plan.publishClean, false); + }); +}); + +describe("action.yml wiring", () => { + const yml = readFileSync(ACTION, "utf8").replace(/\r\n/g, "\n"); + + test("both publication and settlement consult this script — no leftover bash guess", () => { + assert.match(yml, /scripts\/authority\.mjs/, "the planner must be wired"); + assert.match(yml, /publishClean/, "the clean post must honour the planner"); + assert.match(yml, /clearEyes/, "the settle step must honour the planner"); + assert.match(yml, /addThumb/, "the thumb must come from the planner, not a local if"); + // THE BUG. The settle step used to do `[ -n "$ME" ] || ME="github-actions[bot]"` + // after an empty /user, so a custom App's 👀 was never the login it filtered + // on. The fallback lives in this module now, behind the other sources. + assert.doesNotMatch( + yml, + /ME=.*github-actions\[bot\]/, + "action.yml must not hard-code the Actions bot as the settle identity", + ); + }); + + test("the 👀 create carries the author login so settle does not have to guess", () => { + assert.match(yml, /cr-actor/, "create must persist the observed login"); + assert.match(yml, /\.user\.login/, "create reads the identity the POST returned"); + const stale = yml.indexOf("- name: Clear stale run files"); + const cleanup = yml.indexOf("- name: Clean up engine output"); + const react = yml.indexOf("- name: React 👀 to acknowledge review request"); + assert.ok(stale > 0 && cleanup > stale && react > stale, "cleanup steps exist"); + const staleBlock = yml.slice(stale, react); + const cleanupBlock = yml.slice(cleanup); + assert.match(staleBlock, /cr-actor/, "stale-file wipe includes the carried identity"); + assert.match(cleanupBlock, /cr-actor/, "end-of-run wipe includes the carried identity"); + }); + + test("the clean verdict is planned before it is posted, not after in reaction settlement", () => { + const post = yml.indexOf("- name: Post review comments"); + const settleStep = yml.indexOf("- name: Settle the review reactions"); + const publishClean = yml.indexOf("publishClean"); + assert.ok(post > 0 && settleStep > post, "post then settle"); + assert.ok(publishClean > post && publishClean < settleStep, "authority is consulted inside the post step"); + }); +});