diff --git a/.github/workflows/seidroid-review.yml b/.github/workflows/seidroid-review.yml new file mode 100644 index 0000000..e99e339 --- /dev/null +++ b/.github/workflows/seidroid-review.yml @@ -0,0 +1,4061 @@ +name: seidroid review +run-name: UCI / seidroid review / ${{ (github.event.issue.number || github.event.pull_request.number) && format('#{0}', github.event.issue.number || github.event.pull_request.number) || github.ref_name }} +# Reusable, comment-triggered agentic pull request review. The reviewer's logic and +# its prompt live in the sei-agent-driver binary from sei-protocol/sei-internal-skills; +# this file is the GitHub wiring around it, pinned through the `driver-version` +# default. A thin caller in the reviewed repository wires the triggers and calls this +# with `uses:`. +# +# It REPLACES ai-review.yml rather than running beside it: a repository that wires the +# automatic path here should retire that caller in the same change, or every pull +# request is reviewed twice. +# +# The driver's session outlives the run, which is why `mode: close` exists -- it is the +# only thing that reclaims a sandbox. +on: + workflow_call: + inputs: + mode: + description: >- + "review" (default) drives a review turn on the pull request's session, + keeping the conversation so the next invocation can say what changed. + "close" deletes that session instead — the end of the unit of work, and + the only thing that reclaims its sandbox. + required: false + type: string + default: 'review' + driver-version: + description: >- + sei-agent-driver version to install. It names a revision of + sei-protocol/sei-internal-skills, NOT of this repository: the driver is a + nested module there and moving this file did not move it. Passing a uci sha + here fails the install with an unknown revision. + + LEAVE IT UNSET. The default is the release this file drives, and it is what + makes one conclusion mean one thing across callers: a gate written against + `success` in one repository then reads the same in the next. Set it only to + run AHEAD of the default, on a driver change that has shipped and this file + has not yet taken. + + Forward only. The install step below refuses a driver older than the default, + because the conclusion a review reaches for a given set of findings is + specific to the driver that reached it. Pin backward in this repository, + under review, rather than in a caller. + + The module is nested, so the repository carries path-prefixed tags + (sei-agent-driver/vX.Y.Z) while `go install` takes the bare version. Pass + `v0.21.0`; `sei-agent-driver/v0.21.0` is refused as a disallowed version + string. A commit sha resolves to a pseudo-version. + + Verify a pin from an EMPTY module cache: a warm one is a false green, because + it resolves a pin the proxy may never have served. + required: false + type: string + default: 'v0.21.0' + trigger-phrase: + description: >- + The mention a person types to ask for a review. The command is that phrase + followed by `review`, alone on a line, optionally followed by `close`. + + The `@` is optional. `@seidroid review` and `seidroid review` both ask for + the same thing, and a person who types the phrase without the mention still + means it. Two things make the wider form safe. The pattern is anchored to a + whole line, so a comment that discusses the command has other words on the + line and does not match. And ai-assistant.yml claims a body only when it + carries the `@` form, so the bare form reaches this workflow alone and no + second tool answers it. + + It reaches a `grep -E` pattern, so its SHAPE is constrained rather than + escaped: a leading `@` and then letters, digits, `_` and `-` only. None of + those is an ERE metacharacter, so the pattern needs no escaping and a phrase + carrying `.` or `|` cannot widen the match. A phrase outside that shape is + refused with a warning and the default is used, which is what + guidelines-file below does with a name it cannot trust. + + Matched case-sensitively. `@Seidroid review` starts no review. + required: false + type: string + default: '@seidroid' + allowed-team: + description: >- + org/team-slug whose active members may ask for a review by comment. Empty + denies every commenter, which is what ai-review.yml does with the same + input under the same default. + + It gates the comment path alone. An automatic pull_request review has no + commanding actor, so the guard skips this check there and reviews a pull + request opened by anyone. + + The membership read takes "Members" organization permissions, which a + GITHUB_TOKEN cannot carry. Only the App identity can answer it, so a caller + that configures no App is refused on the comment path. Pass SEIDROID_APP_ID + and SEIDROID_APP_PRIVATE_KEY to use that path. Setting this input empty is + not the way out: empty denies. + + Two things still work without the App. An automatic pull_request review + never reaches this check, and neither does `@seidroid review close`. + required: false + type: string + default: 'sei-protocol/sei-core' + allowed-bots: + description: >- + JSON array of exact bot logins that may ask for a review by comment. `[]`, + the default, denies every bot. + + A login is the discriminating control here, and the actor's type is not: a + bot with write access to the calling repository carries MEMBER or + COLLABORATOR like anyone else, so refusing every bot is the only safe reading + of a type. This input names the ones a repository trusts. dependabot and + renovate are the cases it exists for. + + Matched exactly and case-insensitively, twice: once in the guard's job + condition, so an unlisted bot starts no runner, and again in `Admit the + request`. A lookalike login does not pass either. ai-review.yml checks the + same input the same way. + + A listed bot skips the allowed-team check, because a bot is not a team + member. It is held to every other rule: the fork check, the skip label and + the command grammar all apply. + + Malformed JSON is a caller error: `fromJSON` refuses it rather than denying + quietly. `Admit the request` denies on it, so neither reading admits a bot + off a value nobody could parse. + required: false + type: string + default: '[]' + approve-on-success: + description: >- + Approve the pull request when the review concludes clean. Off by + default: clearing a human review requirement is the repository's policy + decision, not this file's, so a clean run stays silent until the + repository has decided it may approve. A blocking conclusion requests + changes either way. + required: false + type: boolean + default: false + publish-ai-review-check: + description: >- + Publish the review's check run a second time, under the name `AI Review`. + On by default. A branch-protection rule matches a check by its name, the + rules on a calling repository are org-level, and no token here reads them. + A repository that retires `ai-review.yml` under a rule that names + `AI Review` waits forever on a check nothing publishes. That rule reads as + pending, not failed. + + Set this false where the repository still runs `ai-review.yml` AND a rule + names `AI Review`. Both tools then publish that name, GitHub lists only the + check run that completed last, and this copy can hide the incumbent's + verdict. This switch does not remove that race. It gives the caller the + choice to accept it or to stand the copy down. + required: false + type: boolean + default: true + skip-review-label: + description: >- + A label on the reviewed pull request that stops the review. Empty + disables the check. + required: false + type: string + default: 'ai: skip-review' + nitpick-label: + description: >- + A label on the reviewed pull request that puts every nit-grade finding on + the line it is about, as a comment thread of its own. Empty disables the + check. + + Without the label a nit is still reported. The driver sends it to the + non-blocking bucket instead, where the verdict comment and the check run + summary both carry it as prose and no thread opens on the code. So this + chooses where a nit lands, not whether the review makes one. + + It also decides what a nit may close. A thread this reviewer left before + is superseded by the comment that restates it, and a nit that opens no + comment supersedes nothing. + + The name matches ai-review.yml's, so a repository running both tools adds + one label rather than two. + + Adding it starts no review. This workflow reviews a pull request once, and + a relabel earns no second one, so label the pull request and then comment + `@seidroid review`. + required: false + type: string + default: 'ai: nitpick' + re-review-on-push: + description: >- + Review again on every push to a pull request this workflow has already + reviewed. Off by default: one automatic review per pull request, and a + person asks for the next one by name. + + Off does not mean exactly one. A review that finds something blocking + leaves a CHANGES_REQUESTED, only a later review withdraws it, so this + workflow reviews every push after that one until a review comes back + clean. Size the saving on that: it covers the pull requests that pass + first time, and a pull request pushed to ten times under a block costs + ten reviews with this off. + + Each review holds a managed sandbox for up to `timeout-minutes` and + spends model quota, so a pull request pushed to ten times costs ten of + each. It also posts ten verdicts. + + A push is the only event either rule covers. Reopening a pull request + or relabelling it changes no code, so neither earns a second review, + whatever this says and whether or not a block stands. + + It governs the automatic path alone. An explicit `@seidroid review` + comment is answered however many reviews have already run. + + Two consequences of leaving it off. The refusal is a notice on the + workflow run: an author who pushes and sees no new review reads nothing + on the pull request that says why, or that a comment asks for one. And + the `review` check run is published against the commit a review read, + so every head after the first carries none -- a repository that + REQUIRES that check would sit pending rather than fail. Turn this on + there, or require the workflow job instead. + required: false + type: boolean + default: false + guidelines-file: + description: >- + The repository's own review standards, read from the base branch and + outranking the driver's checklist. Empty reads REVIEW.md, which is what + ai-review reads and what these repositories keep. Checked before it + reaches a command, so a name carrying a shell metacharacter, an absolute + path or a parent reference falls back to the default. + required: false + type: string + default: '' + extra-instructions: + description: >- + Guidance this repository adds to every review, alongside the standards + above. The one input the review does not treat as data: it comes from + this workflow rather than from the pull request. + required: false + type: string + default: '' + timeout-minutes: + description: >- + Cap on the review job. Keep it comfortably above `run-deadline-seconds`: + the driver reports a timeout and the reason it hit one, where the runner + killing the job leaves an annotation saying only that the job was + cancelled. + + The margin is not decoration. A review costs more wall-clock than its own + deadline -- the scout pass, the sandbox launches, the install and the + publish steps all sit outside it -- so a cap set AT the deadline turns the + driver's own report into a runner cancellation. 60 minutes clears the 1800s + default with room for that overhead. + required: false + type: number + default: 60 + run-deadline-seconds: + description: >- + How long the driver gives one review before it gives up, as + SEIDROID_RUN_DEADLINE_S. Its own default was 1200s, which reviews were + reaching: measured on sei-protocol/platform, two runs exceeded it and + published no verdict at all, while ordinary ones landed at 943s and 1029s + -- 79% and 86% of the ceiling. + + A review that crosses this posts nothing. Not a failing check a reader can + act on: no verdict, no findings, and a pull request that looks unreviewed + rather than looking broken. Raise it before lowering what a review reads. + + Keep `timeout-minutes` above it; the note there says why. + required: false + type: number + default: 1800 + runs-on: + description: >- + Runner label for the review job. Defaults to a GitHub-hosted runner, + which pairs with the https base URL below: the two have to describe one + topology. An in-cluster group instead would reach an internet-facing + NLB from inside its own VPC, which does not hairpin, and would also + queue forever on any repository not in that runner group. + required: false + type: string + default: 'ubuntu-latest' + omnigent-base-url: + description: >- + omnigent base URL. The https ingress by default. It cannot be the + in-cluster ClusterIP Service, which is plain http on port 80: a + credentialed client refuses to be built against it, and the token mint + refuses to send the client secret over it. + + This default is a development deployment. Platform changes land there + first, so a change on it can make a review succeed and post nothing. + Brandon Chatham accepts that exposure: no production deployment carries + the seidroid bundle and a provisioned machine client today. Pass a + production URL from a merge-path caller as soon as one exists. + required: false + type: string + default: 'https://seigent.dev.platform.sei.io' + agent-id: + description: >- + The agent NAME to resolve on the server, as SEIDROID_AGENT_ID. Deployment + specific: the driver compiles a default, and a server that calls its agent + something else makes that default unresolvable -- a live run against the dev + deployment failed with `no agent named "seidroid" on this server`, which + nothing in the caller could correct because this input did not exist. + + Empty leaves the driver's own default in place, so a deployment that matches + it needs nothing here. There is no lookup by alias: the name must match what + the server's agent bundle is called, exactly. + + A mismatch does not present as a configuration error. The name is a join key + with the omnigent.ai/agent label the server stamps on the runner Pod and the + admission policy that mounts the git credential from it, so a runner that + does not match attracts no credential and the review reports that it could + not read the repository. + required: false + type: string + default: '' + + machine-client-id: + description: "OMNIGENT_MACHINE_CLIENT_ID for the in-process client_credentials mint. Mirrors the server's own OMNIGENT_MACHINE_CLIENT_ID. Not secret on its own -- the id alone cannot mint a token." + required: false + type: string + default: 'seidroid' + scouts: + description: >- + Independent readings to gather before the review, as `name=agent`, + comma-separated. Each scout reads the same pull request in its own + session on its own agent bundle, seeing neither the review nor another + scout; the review then verifies their claims against the diff and merges + what holds. A scout naming the review's own agent is refused, as are two + scouts sharing one — neither would be a second opinion. Empty runs the + review alone. Passed to the driver as SEIDROID_SCOUTS. + + Two models read every pull request by default. `xreview-scout-codex` is + the one scout bundle sei-internal-skills carries today, and PLT-1168 + tracks a Cursor bundle. A scout that fails costs the review that reading + and nothing else. The driver turns each failure into a note, hands it to + the review, and the review reports with fewer readers. That note is the + only signal, so a bundle absent from a deployment fails quietly on every + pull request. + + Pass the SAME value on the CLOSE job. Scouts hold sessions of their own, + and close derives which to delete from this value, keyed on the scout + NAME. A caller that omits it on both jobs is safe, because close reads + this same default. A caller that sets it on the review job and omits it + on the close job is not. Close then deletes the default name, and the + configured scout keeps its sandbox running. + + Set `scouts: ''` to review on one model. Do that on a deployment that + does not carry the bundle above, and on a caller that wires no + `mode: close` job. A scout holds a sandbox of its own, and close is the + only thing that reclaims one. A caller with no close job leaks one + sandbox per pull request. + required: false + type: string + default: 'codex=xreview-scout-codex' + claude-model: + description: >- + Model to answer the review on, substituting for the one the agent's spec + names. The default pins Opus 5 with the 1M context window. Set it empty + to leave the spec's own. Passed to the driver as SEIDROID_MODEL. + + The pin exists because the runner otherwise launches on the Claude CLI's + own default, which moves on a base image rebuild with no change here. + + The [1m] suffix is load-bearing, not decoration: it selects the 1M + context build. The unsuffixed id runs the standard window, which a + review of a long diff compacts into sooner and reads less of. + + The server forwards the value as-is and enumerates nothing, so an + unrecognised name is not rejected here or at configuration time -- it + fails at turn start, and the review is the thing that does not happen. + + It applies to the review's own agent only. A scout runs on another agent, + so another harness and another provider, and it keeps its spec's model. + required: false + type: string + default: 'claude-opus-5[1m]' + allow-tools: + description: >- + Comma-separated tool_name values to accept. This deployment does stamp + tool_name (measured), so this allowlist discriminates per tool. Passed + to the driver as SEIDROID_ALLOW_TOOLS. + + Defaults to `Bash,Read` because the review cannot happen without them. + The prompt's first step is a `gh pr diff` command, so an empty allowlist + does not yield a shallower review -- it yields a turn that reports it + could not read the diff. The shell runs inside the agent's own sandbox + against its own gh credentials, and the access control on it is the + trigger: only a sei-protocol developer can write the line that starts a + review. + + `Read` is measured, not assumed. A read inside the agent's working + directory raises no prompt, so the staged diff needs no grant; a read + outside one does, and a recorded run had exactly that refused and spent + three extra tool calls recovering. The diff now stages into the working + directory, so this grant is the belt to that braces. + + Brandon Chatham accepts the unrestricted shell. The review builds and + tests the tree where that is straightforward. A reviewer that cannot + compile can only guess at a finding that needs one. ai-review scopes its + own model to `Read,Bash(gh pr diff:*),Bash(gh pr view:*)` and gives up + that capability. That acceptance covers code from inside the + organisation. + + Fork code sits outside that acceptance, and the guard refuses it. An + explicit `@seidroid review` arrives as a comment or review event in the + base repository, which carries the secrets. Those paths reach a fork's + code unless something stops them. The guard's fork check is what stops + it, on every one of them and on the automatic path. This shell therefore + runs only over code from inside the organisation. Weigh that before you + widen or narrow this list. + required: false + type: string + default: 'Bash,Read' + secrets: + OMNIGENT_MACHINE_CLIENT_SECRET: + description: "omnigent machine-client secret, exchanged in-process for a session bearer. Mirrors the server's OMNIGENT_MACHINE_CLIENT_SECRET_HASH -- the server stores only a digest of this value. The one secret an operator must configure to use this workflow." + required: true + SEIDROID_APP_ID: + description: >- + seidroid GitHub App id. Optional for an automatic review, REQUIRED to ask + for one by comment. + + Without it the review posts as the workflow's own identity, which is correct + but reads as github-actions rather than the bot. And the guard's team check + cannot read organisation membership, so it refuses every `@seidroid review` + comment and says why in the run log. An automatic pull_request review and + `@seidroid review close` still run. + + Changing it changes who withdraws a block. A protected branch that restricts + who may dismiss a review takes the dismissal only from a repository admin or + an actor on that list, whoever recorded the review. So put whichever identity + this workflow runs under on the list, and dismiss any standing block by hand + when you change the credential -- otherwise a later clean run fails on a + dismissal the API refuses. + required: false + SEIDROID_APP_PRIVATE_KEY: + description: "seidroid GitHub App private key, exchanged for an installation token scoped to the calling repository. Never written to an output; the action masks it." + required: false + +permissions: {} + +# The marker that opens every verdict this workflow leaves, in one place. Three +# steps use it: the guard finds a review that already ran, the position step opens +# the review it records with it and matches on it to withdraw an earlier block, and +# the verdict comment opens with it. A workflow-level env reaches every step, so +# those uses cannot drift apart. A step-level key of this name shadows this one; +# there is none. +# +# It scopes all of that to this tool's own work. ai-review posts under the same bot +# identity and marks its reviews differently; its position is not this one's to +# change. +# +# Changing the value strands every blocking review posted under the old one: the +# withdrawal matches on startswith, so a later clean run approves and retracts +# nothing, the pull request stays red for a finding that is gone, and only a human +# can clear it. It also makes every reviewed pull request read as unreviewed to the +# guard. Before changing it, confirm no open pull request carries a +# CHANGES_REQUESTED review or a verdict comment whose body starts with the old +# value. +# +# The no-verdict notice and the inline findings carry their own markers, and +# neither starts with this one. A run that reached no verdict does not read as +# reviewed. +env: + VERDICT_MARKER: "" + # What stamps an inline finding as this tool's, read by every step that has to + # recognise one: the step that writes it, the step that reads the history back, and + # the step that closes a thread. One value, because a drift between them fails in the + # worst direction and with no error -- placement keeps stamping the old marker while + # the other two stop matching it, and every finding duplicates again on a green run. + # + # Beside VERDICT_MARKER rather than in the review job, though only that job reads it. + # Two markers in two scopes is how a third one ends up defined somewhere else again. + # + # Not named MARKER. One step below still carries a step-level MARKER of its own, for + # the no-verdict notice, and a step-level key shadows a workflow-level one silently -- + # so a bare MARKER here would work today and become a trap for whichever step later + # forgot to set its own. + FINDING_MARKER: "" + # What stamps the notice that a review reached NO verdict. Beside the other two for + # the reason they are beside each other: two steps write it -- the notice itself, + # and the verdict step that withdraws a stale one -- and a drift between them leaves + # a "this review did not complete" notice standing on a pull request whose review + # did complete, with nothing anywhere saying so. + NO_VERDICT_MARKER: "" + +jobs: + guard: + # Cheap allowlist + command parse on a hosted runner, no secrets, before any + # in-cluster work spins up and before any credential is minted. The + # author_association gate is the trust boundary: only OWNER/MEMBER/ + # COLLABORATOR can fire it -- an untrusted PR author cannot. + # + # Bots are excluded separately, because association does not exclude them: a + # bot with write access to the calling repository carries MEMBER or + # COLLABORATOR like anyone else. Without this, a bot that quotes the command + # -- one of ours echoing an earlier comment, say -- starts a real review, and + # a bot that echoes its own trigger does so repeatedly. + name: Guard + runs-on: ubuntu-latest + # A step condition cannot read `secrets`, so its presence is tested here and + # read back as `env` below — the same shape the review job uses. + # + # Both halves of the credential, because the mint below needs both and carries no + # continue-on-error. An id with no private key would run that step, fail it, and + # take the job with it -- where the App is optional and the answer is to fall back. + env: + HAS_REVIEWER_IDENTITY: >- + ${{ secrets.SEIDROID_APP_ID != '' && secrets.SEIDROID_APP_PRIVATE_KEY != '' }} + # Set at all because the account default is six hours. The guard only reads + # API state, so a minute is generous. + timeout-minutes: 5 + # Read-only, and only what the guard's checks read. Three reads share it. The + # fork check reads the pull request. The once-per-PR gate reads a review, to find + # a standing block, and a comment, to find a verdict. + # + # That second gate read goes to the ISSUE comments endpoint. GitHub documents it + # as taking either permission, so pull-requests alone serves it. issues: read is + # granted beside it so the read does not rest on that alias. A reaction is the + # stricter case and takes issues alone; the review job says so where it needs it. + # + # Every reader prefers the App identity and falls back to this, so a caller that + # configures no App still gets all four. A refused read costs them differently, + # and both costs are deliberate. The once-per-PR gate fails open: a review runs on + # every push, and only the run log says why. The fork check and the skip-review + # label fail closed: the guard refuses the review rather than run it over code it + # cannot place, or against an author who asked for none. + # + # A caller must grant this workflow at least these two, because a reusable + # workflow may only downgrade what its caller granted. pull-requests: read is + # load-bearing rather than nice to have: the label check reads the pull request + # on every review, including an automatic one on a caller that configures no + # App, and it refuses the review when that read fails. GitHub's default token + # grants contents, packages and metadata only, so a caller that takes the + # default reviews nothing on a private repository. + permissions: + pull-requests: read # the pull request the fork and label checks read, and the gate's reviews + issues: read # the comments the gate reads to find a verdict + # Runs for an automatic pull_request review, for any comment-triggered dispatch, + # review or close, and for an event this workflow does not handle. For a comment + # it decides whether the requester may command this workflow at all; for an + # automatic review it decides whether the pull request is in a state worth + # spending a sandbox on. Routing is the caller's: it reads the body and passes the + # mode. A close arriving as a pull_request event skips the guard, since GitHub's + # own event is the authority. + # + # THREE COMMENT EVENTS carry the command. A comment on the conversation arrives as + # issue_comment, a comment in a diff thread as pull_request_review_comment, and a + # review body as pull_request_review. Each names the requester under a different + # payload key, so every read below takes the comment key or the review key, + # whichever the event populated. On pull_request both are empty and this branch + # does not apply. + # + # The two diff-side events are held to their creating action. A review may also + # arrive `edited` or `dismissed`, and a dismissal replays the body of the review it + # dismisses -- so a caller that wires those types would spend a sandbox re-running + # a review every time somebody dismissed one. issue_comment is left as it stands. + # + # A REQUESTER passes on one of two grounds. A person passes on + # author_association, which admits a collaborator on the repository the request + # was made in and no untrusted pull request author. A bot passes only by exact + # login in allowed-bots, because association does not discriminate a bot: one with + # write access to the calling repository carries MEMBER or COLLABORATOR like + # anyone else. `Admit the request` reads both grounds again. + # + # `allowed-bots || '[]'` because a workflow_call default applies only to an input + # the caller OMITS. A caller passing `allowed-bots: ${{ vars.SOMETHING }}` with + # that variable unset passes the empty string, and fromJSON('') is not `[]` -- it + # fails the expression and takes the run with it. Empty therefore reads as the + # documented default here, which denies every bot. A non-empty value that is not + # JSON still fails, and loudly: that is a caller wiring error, not an omission. + # + # The last clause admits an event this workflow does not handle, so the refusal + # step below can name it. Without it the job is skipped, every job after it is + # skipped, and the run reports success having done nothing. pull_request is + # excluded from that clause: a pull_request close skips this guard deliberately, + # and the review job reads that skip as its own trigger. + # + # The pull_request branch carries no author-association check, matching the path + # this file replaces: the event is the push itself rather than a person's + # command. The fork check in `Admit the request` refuses code from outside the + # organisation, on this path as well as the comment path. + if: >- + ${{ (github.event_name == 'pull_request' && inputs.mode != 'close') || + (((github.event_name == 'issue_comment' && github.event.issue.pull_request != null) || + (github.event_name == 'pull_request_review_comment' && github.event.action == 'created') || + (github.event_name == 'pull_request_review' && github.event.action == 'submitted')) && + (((github.event.comment.user.type || github.event.review.user.type) != 'Bot' && + contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), + github.event.comment.author_association || github.event.review.author_association)) || + ((github.event.comment.user.type || github.event.review.user.type) == 'Bot' && + contains(fromJSON(inputs.allowed-bots || '[]'), + github.event.comment.user.login || github.event.review.user.login)))) || + (github.event_name != 'pull_request' && + !contains(fromJSON('["issue_comment","pull_request_review_comment","pull_request_review"]'), + github.event_name)) }} + outputs: + should_run: ${{ steps.parse.outputs.should_run == 'true' && steps.admit.outputs.admit == 'true' }} + pr_number: ${{ steps.parse.outputs.pr_number }} + comment_id: ${{ steps.parse.outputs.comment_id }} + # Which REST collection carries the reactions on the object that asked. The + # acknowledgement and the verdict reaction both take this path, and it differs + # per event: an issue comment lives under issues/comments, a diff-thread comment + # under pulls/comments. Empty where the request is not a reactable object, which + # is also when comment_id is empty, so a step gated on either skips. + # + # The two collections take different token scopes. GitHub grants + # issues/comments/{id}/reactions to Issues alone and pulls/comments/{id}/reactions + # to Pull requests; the review job holds both. + comment_api: ${{ steps.parse.outputs.comment_api }} + # The id of the object that asked, for the log. Populated on every comment + # event, including the one that carries no reactions endpoint, because this + # answers "which request started this run" rather than "what may this run react + # on". comment_id above answers the second and is held back where nothing can + # reach it; the driver's --trigger-id takes this one. + trigger_id: ${{ steps.parse.outputs.trigger_id }} + steps: + # First, and before the identity mint below, so a caller that mis-wired its + # triggers learns the reason without spending a credential. A refusal here fails + # the job, and a failed step skips every step after it. + # + # The job condition above admits an unsupported event for exactly this step. + # Without the refusal the condition simply would not match, the guard and the + # review job would both be skipped, and the run would report success having + # reviewed nothing -- which reads as "the workflow ran" to anyone looking at the + # checks list. + # + # pull_request_target is named apart because it is the one a caller reaches for + # to make a fork review work. It runs against the BASE repository's secrets with + # a writable token while the pull request's head is what a reviewer wants to + # read. This workflow checks nothing out, so no step here would run fork code + # today; the refusal is the control that does not depend on that staying true. + # ai-review.yml refuses the same event for the same reason. + - name: Refuse an event this workflow does not handle + env: + EVENT_NAME: ${{ github.event_name }} + MODE: ${{ inputs.mode }} + run: | + set -uo pipefail + # The mode axis, refused here for the same reason the event axis is: a caller + # error that nothing names costs a run that reports success having reviewed + # nothing. `mode` is a free-form string -- workflow_call has no enum -- so a + # typo reaches every `inputs.mode == 'review'` test below, matches none of + # them, and the whole workflow no-ops in silence. + # + # A pull_request close reaches neither this step nor this job: it skips the + # guard deliberately, and the review job reads that skip as its own trigger. + # So this sees `review` and the typos, and refuses only the typos. + case "$MODE" in + review|close) ;; + *) + echo "::error::seidroid review cannot be called with mode '$MODE'; it handles 'review' and 'close'" >&2 + exit 1 + ;; + esac + case "$EVENT_NAME" in + pull_request|issue_comment|pull_request_review_comment|pull_request_review) ;; + pull_request_target) + echo "::error::seidroid review refuses pull_request_target: it runs with the base repository's secrets and a writable token over a head this workflow did not check out. Call it from pull_request, issue_comment, pull_request_review_comment or pull_request_review instead. A fork pull request is refused on every one of those, by design" >&2 + exit 1 + ;; + *) + echo "::error::seidroid review cannot be called from '$EVENT_NAME'; it handles pull_request, issue_comment, pull_request_review_comment and pull_request_review" >&2 + exit 1 + ;; + esac + - id: parse + # Every GitHub-supplied value (the request body, the PR number, the + # comment id) comes in through env and is read back as "$VAR" below -- + # never interpolated as ${{ }} directly into the shell script, even for + # the two fields (issue number, comment id) that GitHub happens to + # always populate with integers. Routing all three the same way means + # there is one pattern to audit, not one safe-looking exception. + # + # Each read takes the comment key or the review key, whichever the arriving + # event populated. issue_comment and pull_request_review_comment both carry + # `comment`; pull_request_review carries `review`, and its own id under + # `review.id`. A read that took only one key would leave the body empty on the + # other events, and an empty body parses as no command -- a request that + # vanishes with no reaction and no run to point at. + env: + BODY: ${{ github.event.comment.body || github.event.review.body }} + PR_NUMBER: ${{ github.event.issue.number || github.event.pull_request.number }} + COMMENT_ID: ${{ github.event.comment.id || github.event.review.id }} + EVENT_NAME: ${{ github.event_name }} + TRIGGER_PHRASE: ${{ inputs.trigger-phrase }} + run: | + set -euo pipefail + # An automatic review has no comment to parse: the event IS the request, and + # the job condition above has already established which event this is. It + # reports the same outputs the command grammar below produces, so every + # later step reads one shape and no step needs a second code path. + if [ "$EVENT_NAME" = "pull_request" ]; then + { + echo "should_run=true" + echo "pr_number=$PR_NUMBER" + echo "comment_id=" + echo "comment_api=" + echo "trigger_id=" + } >> "$GITHUB_OUTPUT" + exit 0 + fi + + # Which REST collection carries the reactions on the object that asked, and + # so whether an acknowledgement can land on it at all. GitHub publishes a + # reactions endpoint for an issue comment and for a pull request review + # comment, and none for a pull request REVIEW -- only the GraphQL schema + # makes a review reactable. So a command in a review body starts a review and + # earns no reaction, and the notice below is the signal that it started. + # + # The id goes out empty with it. `Acknowledge the trigger` and `Answer the + # request` both gate on comment_id, and the eyes one posts are the eyes the + # other withdraws: an id with no endpoint to reach would leave a request + # wearing eyes nothing clears. + comment_api="" + case "$EVENT_NAME" in + issue_comment) comment_api=issues/comments ;; + pull_request_review_comment) comment_api=pulls/comments ;; + esac + + # The phrase is caller-settable and reaches the grep pattern below, so its + # SHAPE is constrained here rather than escaped: an optional leading @, then + # letters, digits, `_` and `-`. None of those is an ERE metacharacter, so the + # pattern carries the phrase verbatim and a phrase holding `.` or `|` cannot + # widen the match. Escaping instead would have to cover every ERE + # metacharacter correctly forever; a character class is one thing to read. + # + # A phrase outside that shape falls back to the default with a warning, + # which is what guidelines-file does with a name it cannot trust. The + # alternative is failing the job on every comment the repository receives, + # for a caller mistake that leaves the documented phrase working. + phrase="${TRIGGER_PHRASE#@}" + case "$phrase" in + ""|*[!A-Za-z0-9_-]*) + echo "::warning::trigger-phrase '$TRIGGER_PHRASE' is not an optional @ followed by letters, digits, _ or -; using @seidroid instead" + phrase=seidroid + ;; + esac + + cmd="$(printf '%s' "$BODY" | tr -d '\r')" + # Require a LINE reading `@seidroid review`, optionally `close`, and nothing + # else on it. Anchoring to a whole line is what keeps a comment that merely + # quotes or discusses the command from triggering a review. + # + # The @ is optional, and that is a widening this workflow keeps. A person who + # types the phrase without the mention still means it. Whole-line anchoring + # is what makes it safe: a comment discussing the command has other words on + # the line and does not match. And ai-assistant.yml reserves a body only when + # it carries the @ form, so the bare form reaches this workflow alone. + cmdline="$(printf '%s\n' "$cmd" \ + | grep -m1 -E "^[[:space:]]*@?${phrase}[[:space:]]+review([[:space:]]+close)?[[:space:]]*$" || true)" + if [ -z "$cmdline" ]; then + # A review runs only on the repository the pull request is on, so a + # request that names a repository is named in this run's log. That is all + # this does: an issue_comment run attaches to no pull request, so the + # annotation reaches whoever opens the run and nobody else, and + # ai-assistant.yml still answers the body conversationally, because its + # reservation is the bare command alone. Both match ai-review, where the + # target form was never a review command either. + # + # It earns its line by naming the reason in the one place a person + # debugging "why did my request do nothing" will look. + # + # notice, not error, because a denied request is a notice throughout + # ai-review: an unauthorised actor, an unlisted bot, a draft and an empty + # team all deny at that level. error there is reserved for a caller that + # wired the workflow wrongly. + # + # Matched on the same whole-line anchor the command grammar above uses, + # so the refusal covers exactly the shapes that fall through it. + # + # Captured, and tested for emptiness, rather than read from a `grep -q` + # exit status. -q stops at the first match, which leaves printf writing + # into a closed pipe on a body larger than the pipe buffer; pipefail then + # reads the SIGPIPE as a failed pipeline and the refusal is not written. + # Measured: at 232 kB the -q form reports status 141 and stays silent. + named_repo="$(printf '%s\n' "$cmd" \ + | grep -m1 -E "^[[:space:]]*@?${phrase}[[:space:]]+review([[:space:]]+close)?[[:space:]]+[A-Za-z0-9][A-Za-z0-9._-]*/[A-Za-z0-9][A-Za-z0-9._-]*#[0-9]+[[:space:]]*$" || true)" + if [ -n "$named_repo" ]; then + echo "::notice::seidroid review takes no repository target; a review runs only on the repository the pull request is on" + fi + echo "should_run=false" >> "$GITHUB_OUTPUT" + exit 0 + fi + if [ -z "$comment_api" ]; then + echo "::notice::$EVENT_NAME carries no reactions endpoint, so this request earns no acknowledgement on the object it was written on; the review runs and posts its verdict as a comment" + fi + { + echo "should_run=true" + echo "pr_number=$PR_NUMBER" + # The comment id is passed as --trigger-id, which only labels this + # dispatch in the logs. The pull request, not the comment, is the + # session key — so any dispatch adopts that PR's session and drives a + # fresh review turn on the current tree. + # + # Emitted with the endpoint that reaches it, and held back when there is + # none: the two are read together by every step that reacts. trigger_id + # beside it carries the id whatever the event, so the driver still labels + # a dispatch that can be reacted on nowhere. + if [ -n "$comment_api" ]; then + echo "comment_id=$COMMENT_ID" + else + echo "comment_id=" + fi + echo "comment_api=$comment_api" + echo "trigger_id=$COMMENT_ID" + } >> "$GITHUB_OUTPUT" + + # Only reached once the command itself parsed, so a comment that says + # nothing does not mint a token or call the API. + - name: Mint an identity to ask about the team and the labels + id: identity + if: ${{ steps.parse.outputs.should_run == 'true' && env.HAS_REVIEWER_IDENTITY == 'true' }} + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 + with: + app-id: ${{ secrets.SEIDROID_APP_ID }} + private-key: ${{ secrets.SEIDROID_APP_PRIVATE_KEY }} + owner: ${{ github.repository_owner }} + # Both keys, together. `owner` on its own mints a token that reaches every + # repository the installation is on; `repositories` holds it to this one. + repositories: ${{ github.event.repository.name }} + + - name: Admit the request + id: admit + env: + GH_TOKEN: ${{ steps.identity.outputs.token }} + ALLOWED_TEAM: ${{ inputs.allowed-team }} + ALLOWED_BOTS: ${{ inputs.allowed-bots }} + SKIP_LABEL: ${{ inputs.skip-review-label }} + # Who asked, under whichever key the arriving event populated. A review body + # names its author under `review.user`; the two comment events name theirs + # under `comment.user`. + ACTOR: ${{ github.event.comment.user.login || github.event.review.user.login }} + ACTOR_TYPE: ${{ github.event.comment.user.type || github.event.review.user.type }} + REPO: ${{ github.repository }} + PR: ${{ steps.parse.outputs.pr_number }} + PARSED: ${{ steps.parse.outputs.should_run }} + EVENT_NAME: ${{ github.event_name }} + IS_DRAFT: ${{ github.event.pull_request.draft }} + ACTION: ${{ github.event.action }} + RE_REVIEW_ON_PUSH: ${{ inputs.re-review-on-push }} + # What the CALLER routed this dispatch as. The fork check and the label + # check below both gate on it, and the fork check states why. + MODE: ${{ inputs.mode }} + # The pull_request payload's own repository ids, which spare that path an + # API call. Empty on every other event, where the API answers instead. + HEAD_REPO_ID: ${{ github.event.pull_request.head.repo.id }} + BASE_REPO_ID: ${{ github.event.pull_request.base.repo.id }} + # The App identity where a caller configured one, this workflow's own + # token where it did not. Named apart from GH_TOKEN above, because the + # team check has no such fallback. Reading an organisation's teams takes + # "Members" organization permissions, which a GITHUB_TOKEN cannot carry. + # The reads below need no more than pull-requests: read. + GATE_TOKEN: ${{ steps.identity.outputs.token || github.token }} + run: | + set -uo pipefail + deny() { echo "::notice::$1"; echo "admit=false" >> "$GITHUB_OUTPUT"; exit 0; } + if [ "$PARSED" != "true" ]; then + echo "admit=false" >> "$GITHUB_OUTPUT" + exit 0 + fi + + # A draft is not ready to be read, and on the automatic path every push to + # one would otherwise spend a sandbox for a review nobody asked for. Read + # from the event payload rather than the API, so it needs no identity and + # cannot fail open when none is configured. + # + # The manual path skips this check on purpose: commenting `@seidroid review` + # on a draft is a direct request, and refusing it would be surprising. + if [ "$EVENT_NAME" = "pull_request" ] && [ "$IS_DRAFT" = "true" ]; then + deny "$REPO#$PR is a draft; not reviewing" + fi + + # WHO may command a review, checked again here. The job condition has + # already applied the same two grounds, and this is the second reading of + # them: a condition is one expression on one line, and a control worth having + # is worth stating where a person debugging a refusal can read the reason. + # + # A bot is held to allowed-bots and a person to allowed-team, because neither + # test answers for the other. A bot is not a team member, so the membership + # read would refuse every bot however trusted. And a bot's author_association + # says nothing: one with write access to the calling repository carries MEMBER + # or COLLABORATOR like anyone else. + # + # Both fail closed: empty, malformed and unanswerable all deny. This step + # runs without -e, so a read that fails leaves the variable empty, and an + # empty value matches neither "true" nor "active". + # + # Both gate the comment path only. An automatic run has no commander: + # applying either check there would silently stop reviewing every pull + # request opened by anyone outside the team, which is the opposite of what a + # caller sets these inputs for. + # + # Both stop a REVIEW, not a teardown, for the reason the label check below + # states. Any requester the job condition admits may reclaim a sandbox, + # because the alternative is a sandbox nothing reclaims. + if [ "$EVENT_NAME" != "pull_request" ] && [ "$MODE" != "close" ]; then + # Lowercased before the comparison, so this reader and the job condition + # read one type the same way: GitHub's expression `==` ignores case and + # the shell's does not. + actor_type="$(printf '%s' "$ACTOR_TYPE" | tr '[:upper:]' '[:lower:]')" + if [ "$actor_type" = "bot" ]; then + # An exact, case-insensitive login match against a JSON array, which is + # how ai-review.yml reads the same input. Exact, because a substring test + # admits a lookalike in either direction: `bot` passes against a listed + # `dependabot[bot]`, and so does `dependabot[bot]x`. Case-insensitive, + # because GitHub treats one login as one account whatever case it is + # written in, and the job condition above compares it that way too. + # + # A value that is not a JSON array of strings yields no match and denies. + listed="$(printf '%s' "$ALLOWED_BOTS" \ + | jq -r --arg actor "$ACTOR" \ + 'if type == "array" + then any(.[]; type == "string" and ascii_downcase == ($actor | ascii_downcase)) + else false end' 2>/dev/null || true)" + [ "$listed" = "true" ] || deny "$ACTOR is not in allowed-bots; denying" + else + # Reading an organisation's teams needs the App identity, so a caller + # that configures no App is refused here. The notice says so, and names + # the one thing that fixes it. + case "$ALLOWED_TEAM" in + */*) ;; + *) deny "allowed-team is empty or is not org/team-slug; denying" ;; + esac + [ -n "${GH_TOKEN:-}" ] || deny "this run holds no App identity, so it cannot read membership of $ALLOWED_TEAM; denying. Pass SEIDROID_APP_ID and SEIDROID_APP_PRIVATE_KEY to this workflow. An automatic pull_request review and @seidroid review close do not reach this check" + state="$(gh api "orgs/${ALLOWED_TEAM%%/*}/teams/${ALLOWED_TEAM##*/}/memberships/${ACTOR}" --jq .state 2>/dev/null || true)" + [ "$state" = "active" ] || deny "$ACTOR is not an active member of $ALLOWED_TEAM; denying" + fi + fi + + # A fork pull request carries code from outside the organisation. A review + # clones that code into a sandbox. That sandbox holds a live App credential + # and a shell. Both paths refuse it. ai-review.yml refuses the comment path in + # the same words. + # + # The automatic path needs this as much as the comment path. GitHub withholds + # this workflow's secrets from a fork pull_request run by default. The + # machine-client check below then fails such a run. A private or internal + # repository can turn that withholding off, per repository or by organisation + # policy. This check does not rest on a setting nobody here controls. + # + # Keyed on the EVENT, not on whether the payload carried the ids. A + # pull_request payload carries both, so that path spends no API call. + # issue_comment carries no pull request object at all, so the API answers + # there. pull_request_review_comment and pull_request_review DO carry + # `pull_request.head.repo.id` and `.base.repo.id`, and this branch ignores + # them: the label check below reads the same endpoint unconditionally on + # every one of these paths, so reading the payload here would remove one of + # two identical round trips and neither the failure mode nor the dependency. + # A branch keyed on payload shape rather than on event would also have to + # keep this check's fail-closed rule, and that rule is the highest-consequence + # one in the guard. Repository ids, not names, so a rename does not read as a + # fork. A null head repository reads as a fork, which is the safe reading. + # + # This check fails closed, unlike the label check below. Only a definite "same" + # admits, so an unreadable origin refuses. A failed read costs one refused + # review a person can retry. Admitting on a signal nobody could read costs the + # sandbox above. The step runs without -e, so a failed read leaves the variable + # empty. The API's own error goes to the log, where it says why. + # + # It stops a REVIEW, not a teardown, for the reason the label check states. A + # fork pull request must still be able to reclaim its sandbox. + # + # Keyed on the caller's mode, because the caller is the reader of the comment + # body that routes the dispatch. Two readers of one grammar can disagree: this + # guard's own parse accepts a bare `seidroid review close`, where a caller + # matching the documented `@seidroid` form reads that same comment as a + # review. Mode decides what the review job does, so mode is what the checks + # here gate on. + if [ "$MODE" != "close" ]; then + if [ "$EVENT_NAME" = "pull_request" ]; then + # An empty BASE id means the payload did not carry the signal, which is + # the same standing as a read that failed. It must not compare equal to an + # empty HEAD id and admit. + if [ -z "$BASE_REPO_ID" ]; then + origin=unreadable + elif [ "$HEAD_REPO_ID" = "$BASE_REPO_ID" ]; then + origin=same + else + origin=fork + fi + refusal="$REPO#$PR is fork-originated; not reviewing it" + else + origin="$(GH_TOKEN="$GATE_TOKEN" gh api "repos/$REPO/pulls/$PR" \ + --jq 'if .head.repo.id != null and .head.repo.id == .base.repo.id then "same" else "fork" end' \ + || true)" + refusal="explicit re-reviews are disabled for fork-originated pull requests; not reviewing $REPO#$PR" + fi + case "$origin" in + same) ;; + fork) deny "$refusal" ;; + *) deny "could not read where $REPO#$PR comes from, so a fork cannot be ruled out; not reviewing it" ;; + esac + fi + + # The label stops a REVIEW, not a teardown. A pull request that gains the + # label after a session exists must still be able to reclaim its sandbox, and + # nothing else will: no lifetime cap, no sweep. Keyed on mode for the reason + # the fork check states. + # + # It reads under GATE_TOKEN, so the label bites for a caller that configures + # no App. GitHub documents GET /repos/{owner}/{repo}/pulls/{n} as taking + # "Pull requests" read or "Contents" read, and this job grants the first. + # + # It fails CLOSED, like the fork check above and unlike the once-per-PR gate + # below. A refusal costs one review, and the notice names the two fixes. + # Admitting on a read that did not answer costs the label its meaning, on the + # one pull request whose author asked for no review. Anything but a plain + # `false` denies. + if [ "$MODE" != "close" ] && [ -n "$SKIP_LABEL" ]; then + # shellcheck disable=SC2016 # $ENV is jq's own, and jq reads it + carries="$(GH_TOKEN="$GATE_TOKEN" \ + gh api "repos/$REPO/pulls/$PR" \ + --jq 'any(.labels[]?.name; . == $ENV.SKIP_LABEL)')" \ + || deny "could not read the labels on $REPO#$PR, so $SKIP_LABEL cannot be ruled out; not reviewing. Grant pull-requests: read on the calling job, or pass SEIDROID_APP_ID and SEIDROID_APP_PRIVATE_KEY" + if [ "$carries" != "false" ]; then + deny "$REPO#$PR carries $SKIP_LABEL; not reviewing" + fi + fi + + # One automatic review per pull request. What records that one already ran + # is the verdict this workflow published, and it publishes that in one of + # two places: the review carrying the inline comments where that review + # landed, and a comment of its own where it did not. Either hangs on the + # pull request rather than on a commit, and a push does not remove it. The + # review check run is keyed on the head commit, so a push leaves none behind + # and every push would read as a first review. + # + # Reading only the comments was correct while the verdict was always one. + # It is not now: the run that places findings on the diff carries its verdict + # in that review and posts no comment at all, and a gate reading comments + # alone would find nothing and review again on the next push -- and on every + # push after it. + # + # The comment path never reaches this. A person who asks for a review by + # name is answered, however many have already run. + # + # Last of the checks, and the only one that pages the API. It makes at most + # three reads, and each one found skips the rest. + if [ "$EVENT_NAME" = "pull_request" ]; then + re_review=false + if [ "$ACTION" = "synchronize" ] && [ "$RE_REVIEW_ON_PUSH" = "true" ]; then + re_review=true + fi + # A standing block exempts a PUSH to the pull request. The withdrawal lives + # inside a later review, so a gate that refuses every automatic run also + # refuses the only run that can retract a CHANGES_REQUESTED this workflow + # left. The author pushes the fix and the block stands, clearable by a + # comment they were never told to write. + # + # Scoped to `synchronize` for the reason the input above is: reopening a + # pull request or relabelling it changes no code, so a review of it reaches + # the finding the block already names. The block is still accurate, and the + # next push is what clears it. + # + # A dismissed review reads as DISMISSED, so what this finds is a block that + # is still standing. + # + # It has to select what the WITHDRAWAL will actually dismiss, or this gate + # opens a review that cannot close what it opened for -- and then does it + # again on the next push, and every push after it, because the block it + # names never goes away. The marker alone selected, so a person who opened + # their blocking review by quoting this tool's previous verdict bypassed + # the once-per-pull-request gate for the life of the pull request. + # + # Held to the Bot test, which is the one the prior-verdict read below + # applies and the one that removes that case: a person is not a Bot. It is + # deliberately WIDER than the withdrawal's own test, which names the two + # logins exactly. The two fail in opposite directions and each is pointed + # the safe way: this one admits a review to go and look, costing a run, + # where the withdrawal refuses to mutate a review it does not own. What + # remains is another BOT opening its body with this tool's private marker, + # which is the same residue the prior-verdict read accepts. + if [ "$re_review" = "false" ] && [ "$ACTION" = "synchronize" ]; then + # A read that fails cannot rule a block out, so the gate admits and lets + # the review look. That costs one review. A refusal costs the withdrawal + # of a merge gate this workflow itself left standing, which then needs a + # human. The API's own error goes to the log, where it says why. + read_ok=true + # shellcheck disable=SC2016 # $ENV is jq's own, and jq reads it + blocked="$(GH_TOKEN="$GATE_TOKEN" \ + gh api "repos/$REPO/pulls/$PR/reviews?per_page=100" --paginate \ + --jq '.[] | select(.user.type == "Bot" and .state == "CHANGES_REQUESTED" + and ((.body // "") | startswith($ENV.VERDICT_MARKER))) | .id')" || read_ok=false + if [ -n "$blocked" ]; then + echo "::notice::$REPO#$PR carries a block from this workflow, so this review runs to withdraw it" + re_review=true + elif [ "$read_ok" = "false" ]; then + echo "::warning::could not read the reviews on $REPO#$PR, so a block this workflow left cannot be ruled out; this review proceeds to withdraw one" + re_review=true + fi + fi + + if [ "$re_review" = "false" ]; then + # The author is tested by type rather than by login: the verdict + # posts under the App identity where a caller configured one and + # under this workflow's own where it did not. The marker at the + # top of this file must open the body. A person quoting it is not + # a Bot, and a bot that mentions it does not open with it, so + # neither reads as a verdict. + # + # It fails open, where the label check above refuses. A read that + # fails costs one extra review, and the next push corrects it. A + # refusal on a signal this step could not read costs the review + # itself, on a pull request whose author never learns it was + # refused. + # + # A full page at a time: the pull request this gate matters most + # on is the one pushed to ten times, which is also the one carrying + # the most comments to page through. The page size rides in the + # path because `-F` on a `gh api` that names no `-X` makes the + # request a POST, and this one only ever reads. + # + # A verdict found is a verdict found, whatever the read did afterwards. + # --paginate streams a page at a time, so a read that finds one on page 1 + # and then meets a 5xx on page 3 exits non-zero holding it -- and the pull + # request that pages is the one this gate is worth most on. + read_ok=true + # The reviews first, because that is where a review that reached the diff + # leaves its verdict, and because it is the shorter list on the pull + # request this gate is worth most on. Both tests are the ones the comment + # read below applies, for the reasons given there. + # shellcheck disable=SC2016 # $ENV is jq's own, and jq reads it + prior="$(GH_TOKEN="$GATE_TOKEN" \ + gh api "repos/$REPO/pulls/$PR/reviews?per_page=100" --paginate \ + --jq '.[] | select(.user.type == "Bot" and ((.body // "") | startswith($ENV.VERDICT_MARKER))) | .id')" || read_ok=false + # The comments, for the run that placed nothing on the diff and so posted + # its verdict on its own. Skipped once the reviews answered: the question + # is whether a verdict exists, and one found is one found. + if [ -z "$prior" ]; then + # shellcheck disable=SC2016 # $ENV is jq's own, and jq reads it + prior="$(GH_TOKEN="$GATE_TOKEN" \ + gh api "repos/$REPO/issues/$PR/comments?per_page=100" --paginate \ + --jq '.[] | select(.user.type == "Bot" and ((.body // "") | startswith($ENV.VERDICT_MARKER))) | .id')" || read_ok=false + fi + if [ -n "$prior" ]; then + deny "$REPO#$PR already carries a verdict from this workflow, and this $ACTION event does not earn another; comment @seidroid review to ask for one" + elif [ "$read_ok" = "false" ]; then + echo "::warning::could not read the reviews or the comments on $REPO#$PR, so a review that already ran cannot be found; this one proceeds" + fi + fi + fi + + echo "admit=true" >> "$GITHUB_OUTPUT" + + # Fail here rather than after the in-cluster job is scheduled. A caller that + # forgot the secret is the most likely misconfiguration, and `required: true` + # does not catch it: a caller passing an unset secret satisfies "provided" + # with an empty string. Without this the driver would spin a runner, mint + # nothing, and report a credential error several minutes later. + # + # Only ever tests emptiness -- the value is never echoed, compared against a + # literal, or written to an output. + # + # Reads the verdict as well as the parse. `deny` exits 0, so every step after it + # still runs. A refused request needs no credential, and a fork pull_request run + # holds none: without the verdict here, refusing one would paint a deliberate + # refusal red and name a caller misconfiguration that does not exist. + - name: Require the machine-client secret + if: steps.parse.outputs.should_run == 'true' && steps.admit.outputs.admit == 'true' + env: + SECRET_PRESENT: ${{ secrets.OMNIGENT_MACHINE_CLIENT_SECRET != '' }} + run: | + if [ "$SECRET_PRESENT" != "true" ]; then + echo "::error::OMNIGENT_MACHINE_CLIENT_SECRET is not set on the calling repository; configure it as a repository or organization secret and pass it to this workflow" >&2 + exit 1 + fi + + # Half a credential mints nothing, and the review runs under the workflow's own + # identity instead of failing. That is the better outcome and a silent one, so + # this says it out loud. Not fatal: the App is optional, and a review under + # github-actions is a review. + # + # Either half alone, not just a missing key. A diagnostic with a blind spot + # sends the reader looking at the half that is already set. + # + # Step-level env, like the check above. A step `if` cannot read `secrets`; a + # step `env` can. + - name: Report a half-configured reviewer identity + if: steps.parse.outputs.should_run == 'true' + env: + APP_ID_PRESENT: ${{ secrets.SEIDROID_APP_ID != '' }} + APP_KEY_PRESENT: ${{ secrets.SEIDROID_APP_PRIVATE_KEY != '' }} + run: | + missing="" + if [ "$APP_ID_PRESENT" = "true" ] && [ "$APP_KEY_PRESENT" != "true" ]; then + missing=SEIDROID_APP_PRIVATE_KEY + elif [ "$APP_KEY_PRESENT" = "true" ] && [ "$APP_ID_PRESENT" != "true" ]; then + missing=SEIDROID_APP_ID + fi + if [ -n "$missing" ]; then + echo "::warning::$missing is not set beside the other half of the App credential, so this review posts as github-actions[bot] rather than as the App. Pass both secrets for the App identity, or unset the half that is set" + fi + + review: + name: Review + needs: guard + # A close arriving as a pull_request event needs no guard verdict — GitHub's own + # event is the trigger — so it runs even though the guard was skipped. always() + # is required: a skipped dependency would otherwise skip this too. + # + # A close asked for in a COMMENT is a different thing and does need one. The + # guard admits it on two things only: the comment grammar it parses, and the + # OWNER/MEMBER/COLLABORATOR filter on the job condition. The team, fork and + # label checks all exempt a teardown, because a close a check refuses leaves a + # sandbox that nothing else reclaims. + # !cancelled() rather than always(), and the guard's RESULT rather than only its + # output. always() started this job when the guard had failed -- the secret check + # is the last thing the guard does, so should_run is already set by then and the + # fast-fail bought nothing -- and started it on a cancelled run, which spends + # model quota and holds a sandbox for a review nobody is waiting for. + # + # !cancelled() is still needed because the close path SKIPS the guard, and a + # skipped dependency would otherwise skip this too. Each path names the guard + # result it expects, so neither admits the other's: a pull_request REVIEW is + # guarded, because that is where draft and skip-label are decided, while a + # pull_request CLOSE is not guarded at all. + # + # The command arrives on three comment events, and all three name the guard's + # success. An event this workflow does not handle fails the guard rather than + # skipping it, so no clause here admits one. + if: >- + ${{ !cancelled() && ( + ((github.event_name == 'issue_comment' || + github.event_name == 'pull_request_review_comment' || + github.event_name == 'pull_request_review' || + (github.event_name == 'pull_request' && inputs.mode == 'review')) && + needs.guard.result == 'success' && + needs.guard.outputs.should_run == 'true') || + (github.event_name == 'pull_request' && + inputs.mode == 'close' && + needs.guard.result == 'skipped')) }} + # Exactly one review per PR: a newer `seidroid review` cancels an in-flight one + # (latest wins, never two posters); the driver traps cancellation and STOPS its + # session, keeping the conversation. Only the close event deletes it, so a rapid + # re-trigger does not reclaim the sandbox. Job-level so the group is entered only + # when a real command runs. + concurrency: + # The mode is in the group: without it a review dispatch and a close dispatch + # for one pull request share a group under cancel-in-progress, so commenting on + # a just-closed pull request cancels the in-flight close -- and close is the + # only thing that reclaims a sandbox. + # + # What it costs is the other direction: a review and a close for one pull request + # land in different groups and can run at once, so a close can delete the session + # a review turn is using. Whether that is safe is sei-agent-driver's answer and + # not this file's, which is why the group still reads this way. PLT-1274. + # + # The event's own number is the third term's fallback, and it is load-bearing: + # a close arriving as a pull_request event skips the guard, so the guard's + # number is empty there. Without it every reclaim run shares one group, and + # two closes for different pull requests cancel each other -- while close is + # the only thing that reclaims a sandbox. + group: >- + seidroid-review-${{ inputs.mode }}-${{ github.repository }}-${{ needs.guard.outputs.pr_number || github.event.pull_request.number }} + cancel-in-progress: true + # The reviewed repo's runner label, GitHub-hosted by default. The default pairs + # with the https base URL: the mint refuses to send the client secret over plain + # http, so the in-cluster ClusterIP Service is not a usable target however the + # runner is hosted. + runs-on: ${{ inputs.runs-on }} + # The outer bound on a wedged review. Without it the account default is six + # hours, and the managed sandbox stays up for the whole of it. Comfortably + # above the driver's own SEIDROID_RUN_DEADLINE_S so the driver reports the + # timeout, with its reason, before the runner kills the job. + timeout-minutes: ${{ inputs.timeout-minutes }} + permissions: + pull-requests: write # post the verdict comment and the review position + contents: read # read PR metadata + checks: write # publish the review's check runs + # React to the triggering comment. BOTH scopes are load-bearing, one per + # collection: a conversation comment's reactions live under issues/comments, + # whose POST and DELETE GitHub's permission table grants to Issues alone, and a + # diff-thread comment's under pulls/comments, granted to Pull requests. The + # guard picks the collection per event, so a review that reacts on a diff thread + # needs pull-requests: write and one on the conversation needs issues: write. + # Prune either and the reaction fails on the path it serves, silently: all three + # reacting steps treat a lost reaction as a courtesy and warn. + # + # The comment itself is grantable by either scope, so the alias stops at the + # reaction. GraphQL addReaction is the other route, and GitHub documents no + # permission for any mutation, so only a live call settles what that route needs. + issues: write # acknowledge the trigger with a reaction + # The credential lives ONLY here, at job level. It must never be re-declared + # as step-level env on a `uses:` step (composite/action steps do not receive + # step-level env at all) -- that is the exact defect that broke every real + # run of the Python driver's workflow. Every step in this job inherits it. + env: + # Empty is not the same as unset for this one: an empty SEIDROID_AGENT_ID would + # override the driver's default with nothing. envOr treats empty as absent, so + # passing it through empty is safe and keeps one place deciding the default. + SEIDROID_AGENT_ID: ${{ inputs.agent-id }} + OMNIGENT_MACHINE_CLIENT_ID: ${{ inputs.machine-client-id }} + OMNIGENT_MACHINE_CLIENT_SECRET: ${{ secrets.OMNIGENT_MACHINE_CLIENT_SECRET }} + # Whether the bot's identity is available, as a value a step condition can + # read. The secrets context is not one of those -- a step `if` that touches + # it is a workflow-file error, not a false condition -- so the presence test + # happens here, the same way the machine-client check below does it. + # + # Both halves, for the reason the guard's copy states. + HAS_REVIEWER_IDENTITY: >- + ${{ secrets.SEIDROID_APP_ID != '' && secrets.SEIDROID_APP_PRIVATE_KEY != '' }} + steps: + # First, deliberately: the reaction is the only signal the trigger was + # seen, and everything after it -- toolchain, driver install, session + # start -- runs for minutes before anything else appears on the pull + # request. An acknowledgement that arrives after the verdict is not one. + # + # Comment path only. An automatic pull_request review has no comment to + # react to, so the guard leaves comment_id empty and this is skipped. + # + # continue-on-error: an acknowledgement is a courtesy. Failing the review + # because a reaction did not post would trade the whole job for the + # signal that the job started. + - name: Acknowledge the trigger + # A review turn, and a comment to react to. `Answer the request` withdraws this + # reaction and it runs on the same two facts, so a mode it skips must not react + # here: a close would leave eyes that nothing clears. + if: ${{ inputs.mode == 'review' && needs.guard.outputs.comment_id != '' }} + continue-on-error: true + env: + GH_TOKEN: ${{ github.token }} + REPO: ${{ github.repository }} + TRIGGER_ID: ${{ needs.guard.outputs.comment_id }} + # The collection the request's own reactions live under, from the guard. A + # conversation comment and a diff-thread comment take different endpoints, + # and the guard emits an id only where one of them reaches it. + COMMENT_API: ${{ needs.guard.outputs.comment_api }} + run: | + set -euo pipefail + # Reactions are idempotent per (user, content): re-running a review on + # the same comment returns the existing reaction rather than adding a + # second one, so a retry needs no cleanup. + # + # `Answer the request` withdraws this reaction on every path it takes, and the + # last step of the job withdraws it on the one path that step skips. A review + # that keeps it reads as a review that is still running. All three read + # COMMENT_API, so all three reach the collection this one posted to. + if gh api -X POST "repos/$REPO/$COMMENT_API/$TRIGGER_ID/reactions" \ + -f content=eyes >/dev/null 2>&1; then + echo "acknowledged comment $TRIGGER_ID" + else + echo "::warning::could not react to comment $TRIGGER_ID; the review continues" + fi + + - name: Set up Go + uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + # At or above what the driver's own go.mod requires. A live run proved why + # this cannot lag: pinned at 1.24 while the module declared go 1.25.0, the + # install died with + # requires go >= 1.25.0 (running go 1.24.13; GOTOOLCHAIN=local) + # because setup-go exports GOTOOLCHAIN=local, so the go command may not + # fetch the toolchain the module asks for. The step that installs the driver + # sets GOTOOLCHAIN=auto for that reason and would have recovered this on its + # own -- but a version resolved here rather than downloaded there is one + # fewer network dependency on the path, so both are set. + # + # go-version-file is not usable: this workflow checks nothing out, so there + # is no go.mod on disk to read. Bump this when the driver's does. + go-version: '1.25' + + - name: Install the review driver + id: build + shell: bash + env: + DRIVER_VERSION: ${{ inputs.driver-version }} + # Read by the floor below, which a close is exempt from. + MODE: ${{ inputs.mode }} + # Overrides the GOTOOLCHAIN=local that setup-go exports, for the reason that + # step gives above. + GOTOOLCHAIN: auto + # The oldest driver this file drives, and deliberately the same release as + # the driver-version default: a caller may run ahead of that default, never + # behind it. The conclusion a review reaches for a given set of findings is + # specific to the driver that reached it -- v0.12.0 concludes `success` where + # v0.11.0 concludes `neutral`; v0.14.0 writes a `failure` check for a run that + # reaches no verdict where v0.13.0 writes none; v0.15.0 carries the threads a + # re-review closes where v0.14.0 carries none; v0.16.0 bounds the prior-thread + # history by bytes where v0.15.0 bounds it by a count; and v0.17.0 names which + # thread each finding replaces, which is what lets the resolve step below close + # a thread only once its own replacement reached the code. A merge gate keyed on + # one of those is wrong for the others, so this file serves one and refuses the + # rest. + # + # Move this with the driver-version default above: one value in two places, + # and nothing enforces it. The two mistakes are not symmetric. Raising this + # alone fails every caller that omits the input, at once and in the open. + # Raising the default alone leaves a floor that goes on admitting a driver + # this file no longer drives -- the drift the whole check exists to catch, and + # the direction that says nothing while it happens. + # Two releases are load-bearing in a way the list above does not cover, and + # the floor is the later of them. The position step submits `.decision` from + # the check file, only v0.20.0 and later write it, and a file without it costs + # the review its position AND its withdrawal. v0.21.0 then renders the verdict + # from the structured block alone -- summary first, inline findings counted in + # their section rather than repeated, nits folded -- and that body is what the + # placement step below publishes as the review carrying the inline comments. + # An older driver publishes the model's prose there instead: a body written to + # stand beside the findings, posted as the thing that holds them. + # + # An older driver reaching the review path would take all of that silently, + # which is why the floor is the gate rather than a default. + MIN_DRIVER_VERSION: 'v0.21.0' + run: | + set -euo pipefail + # An input default applies only when the caller omits the key. A caller that + # passes the key through from its own optional input sends an empty string, + # which reaches go install as a bare trailing @. The floor is the default, so + # falling back to it here keeps the two the same value by construction rather + # than by the comment below asking. + DRIVER_VERSION="${DRIVER_VERSION:-$MIN_DRIVER_VERSION}" + + # sei-internal-skills is public, so there is no credential and no + # GOPRIVATE. The module path below is absolute and did not change when this + # file moved: the driver stays there. + # + # The driver is a NESTED module. The repository carries path-prefixed tags + # (sei-agent-driver/vX.Y.Z) and `go install` refuses one as a disallowed + # version string; what it takes is the bare version, `v0.20.0`. A sha becomes + # a pseudo-version. + out="$RUNNER_TEMP/bin" + GOBIN="$out" go install \ + "github.com/sei-protocol/sei-internal-skills/sei-agent-driver/cmd/sei-agent-driver@${DRIVER_VERSION}" + bin="$out/sei-agent-driver" + echo "bin=$bin" >> "$GITHUB_OUTPUT" + + # What go install resolved, read from the build info the toolchain stamped + # rather than from the binary's own --version, so a driver that changed how + # it reports itself still answers this. A sha pin reads here as the + # pseudo-version it became, so the log names exactly what reviewed. + version="$(go version -m "$bin" | awk '$1 == "mod" { print $3; exit }')" + echo "installed sei-agent-driver $version" + + # THE CONTRACT CHECK. It fails here, before the driver holds a credential, + # opens a session or spends model quota -- a driver this file cannot drive + # should cost an install, not a review. + # + # Too old is the version floor above, and it is the review's alone. A close + # reaches no conclusion: it deletes the session, and it is the only thing that + # reclaims a sandbox, so holding it to the vocabulary would strand a live + # sandbox behind a caller edit. A close runs on whatever the caller pinned. + # The surface check below still covers it, and `--close` is the part of that + # surface this path needs. + # + # A pseudo-version carries the tag it follows, so a sha sorts against the + # floor the same way a release does, and semver puts a pre-release below its + # own core version. + at_least() { + printf '%s\n%s\n' "$1" "$2" | awk ' + NR == 1 { split(substr($0, 2), got, /[-.+]/); pre = index($0, "-") > 0; next } + { split(substr($0, 2), want, /[.]/) + for (i = 1; i <= 3; i++) { + if (got[i] + 0 > want[i] + 0) { exit 0 } + if (got[i] + 0 < want[i] + 0) { exit 1 } + } + exit pre }' + } + if [ "$MODE" != "close" ] && ! at_least "$version" "$MIN_DRIVER_VERSION"; then + echo "::error::driver contract: sei-agent-driver $version is older than $MIN_DRIVER_VERSION, and reaches a different conclusion for the same findings; drop driver-version from the caller to take this workflow's own default" + exit 1 + fi + + # Too new, or simply not this driver: a renamed subcommand or a renamed flag + # is a contract this file no longer speaks. `review --help` is the whole + # surface this file drives and the only part of it a check can read without a + # credential and without a session. What --help cannot see -- the SEIDROID_* + # variable names and the check.json fields -- still fails at run time. + if ! usage="$("$bin" review --help 2>&1)"; then + echo "::error::driver contract: sei-agent-driver $version has no \`review\` subcommand, which this workflow invokes" + exit 1 + fi + # Every long flag the help names, wherever it sits on the line. Anchoring to + # the start of the line reads only ` --out string`, and cobra prints a flag + # that has a shorthand as ` -o, --out string` -- so adding a shorthand, which + # takes nothing away, would report the flag as missing and fail every review. + supported="$(printf '%s\n' "$usage" | grep -oE -- '--[a-z0-9-]+' \ + | sed 's/^/ /; s/$/ /' | tr -d '\n')" + missing="" + for flag in --out --findings-out --check-out --close --conversation-context \ + --guidelines-file --extra-instructions --include-nits \ + --trigger-id --base-standards-file --base-standards-ref; do + case "$supported" in (*" $flag "*) ;; (*) missing="$missing $flag" ;; esac + done + if [ -n "$missing" ]; then + echo "::error::driver contract: sei-agent-driver $version does not accept \`review\`$missing, which this workflow passes" + exit 1 + fi + + - name: Mint the reviewing identity + # A review is the bot's work, and the identity on it is what a reader + # trusts, so the posting steps below prefer this token to the workflow's own. + # + # Optional on purpose. Without the app credentials every step below falls + # back to the workflow's own token -- correct, just attributed to + # github-actions rather than to the bot. + id: identity + # Minted before the review rather than after it: the step below reads the + # threads this reviewer left last time, and that read needs the same identity + # that wrote them. Gating on a verdict is no longer possible here and no + # longer needed -- an unused token costs one API call, and every step that + # consumes it still carries its own condition. + if: ${{ inputs.mode == 'review' && !cancelled() + && env.HAS_REVIEWER_IDENTITY == 'true' }} + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 + with: + app-id: ${{ secrets.SEIDROID_APP_ID }} + private-key: ${{ secrets.SEIDROID_APP_PRIVATE_KEY }} + owner: ${{ github.repository_owner }} + # Both keys, together, for the reason the guard's mint states. + repositories: ${{ github.event.repository.name }} + + - name: Name the identity this review posts under + # The logins every step below compares a comment's author against, so a thread + # this tool wrote is told apart from one that merely quotes its marker. One + # place, for the reason FINDING_MARKER is one place: two steps computing them + # from expressions of their own drift, and a drift here reads as a pull request + # this reviewer has never touched. + # + # Two of them, because reading and mutating do not carry the same risk. + # REVIEWER_LOGIN is the identity this run uses and is the only one allowed to + # have written a thread this run closes. WORKFLOW_LOGIN is what a run without + # App credentials posts as, and the history read admits it too: a repository + # that has moved between the two would otherwise lose its whole history, and a + # review that re-reports every finding it already made and can close none of + # them is worse than one that simply closes none. + # + # It cannot ride in the job's env block. That is evaluated before any step runs + # and the app slug is a step output, so this exports to $GITHUB_ENV instead and + # the steps below read a plain variable. + # + # Without app credentials the identity step is skipped and every step here falls + # back to the workflow's own token, which posts as github-actions. + # + # continue-on-error, like the other steps that improve publishing without being + # allowed to prevent reviewing. Every reader treats an unset value as matching no + # author, so a failure here costs the history and the thread closing, and says so + # rather than closing the wrong thread. + if: ${{ inputs.mode == 'review' && !cancelled() }} + continue-on-error: true + shell: bash + env: + APP_SLUG: ${{ steps.identity.outputs.app-slug }} + run: | + set -euo pipefail + # The identity a run without App credentials posts under. Written here rather + # than spelled again in each reader, so the two logins come from one place. + workflow_login="github-actions[bot]" + login="$workflow_login" + if [ -n "${APP_SLUG:-}" ]; then login="${APP_SLUG}[bot]"; fi + echo "REVIEWER_LOGIN=$login" >> "$GITHUB_ENV" + echo "WORKFLOW_LOGIN=$workflow_login" >> "$GITHUB_ENV" + echo "this review reads and posts as $login" + + - name: Read the threads this review left before + id: threads + # What this reviewer said last time, and what the author said back. Read + # here rather than by the agent for the reason the diff is: a step the agent + # must perform is a step it can skip, and prose the author controls should + # not travel through a shell to get here. + # + # Its own threads only, on two tests, and both have to hold. + # + # The marker has to OPEN the body, not merely appear in it. Placement writes it + # as the first bytes, and every verdict-side reader in this file keys the same + # way. A contains test admits any comment that quotes the marker instead -- and + # ai-review posts under the same bot identity, so one of its inline comments + # quoting this file's FINDING_MARKER line, on the repository that defines it, + # would pass an author test as well and be read back as a finding this tool + # left. + # + # The author has to be one this tool posts as: this run's identity, or the + # workflow token's. Either, here, because reading is inert; the step that + # closes a thread takes this run's identity alone. + # + # The thread id travels with each one. It is minted here, when the comment + # posts, and the driver's session has no way to learn it -- so this read is + # where a review gets the handle it names to close a finding it has addressed. + # + # Every thread on the pull request, not the first page of them. What this step + # hands over is the whole set; which of it a prompt can afford is the driver's + # budget to spend, and it reports what it leaves out. A cap here would be a + # second, silent one underneath that. + # + # continue-on-error, and the driver reads an absent file as a first review: + # a history that cannot be fetched must cost the recall, not the review. + if: ${{ inputs.mode == 'review' && !cancelled() }} + continue-on-error: true + shell: bash + env: + GH_TOKEN: ${{ steps.identity.outputs.token || github.token }} + REPO: ${{ github.repository }} + PR: ${{ needs.guard.outputs.pr_number }} + THREADS: ${{ runner.temp }}/review-prior-threads.json + run: | + set -euo pipefail + echo "threads_path=$THREADS" >> "$GITHUB_OUTPUT" + # Emptied before anything is written, and the output above is already set. + # RUNNER_TEMP survives between jobs on a non-ephemeral self-hosted runner, + # which is the steady state here, and this name carries no run id. Without + # this, a fetch that fails aborts the step with the OUTPUT published and the + # file untouched -- so the drive step's `[ -s "$THREADS" ]` passes on a file + # ANOTHER pull request's review left, and the driver is handed that pull + # request's findings as this one's own history. Silent both ways: the failure + # is tolerated, and a populated file reads as a successful recall. + # + # The same guard the base-standards read, the drive step and placement each + # state for the same premise. This was the one read path without it. + rm -f "$THREADS" "$THREADS.raw" + owner="${REPO%%/*}"; name="${REPO##*/}" + + # Every page, not the first hundred. A pull request reviewed enough times + # carries more threads than one page holds, and the ones past the cut were + # silently absent -- so a re-review could not see a finding it had already + # made there, and made it again. That is the duplicate thread PLT-1145 + # removed, reached by a different route. + # + # --paginate needs the query to take $endCursor and to return pageInfo, which + # is the whole of what gh asks for. It writes one JSON document per page, and + # `jq -s` is what turns them into one array. --slurp would do the same inside + # gh; jq -s is used instead because it asks nothing of the runner's gh version. + # + # The root comment and the recent ones are fetched as two connections, because + # they answer different questions and one window cannot serve both. Every + # ownership test reads the ROOT -- the marker has to open its body and its + # author has to be this tool -- so that one is `first: 1` and cannot move. The + # replies that matter are the NEWEST, which is what a session has no way to + # know, so that one is `last`. A single `first: 20` gave the oldest twenty of a + # busy thread and the driver then showed the last three of those: three replies + # from the middle of the conversation, presented as the latest word on it. + # + # totalCount rides along so a thread with more replies than this reads can say + # so, rather than being quietly shortened. + # $owner and friends are GraphQL variables, so the query stays literal. + # shellcheck disable=SC2016 + gh api graphql --paginate -f query=' + query($owner: String!, $name: String!, $number: Int!, $endCursor: String) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + reviewThreads(first: 100, after: $endCursor) { + nodes { + id + isResolved + path + line + originalLine + root: comments(first: 1) { nodes { id body author { login } } } + recent: comments(last: 20) { + totalCount + nodes { id body author { login } } + } + } + pageInfo { hasNextPage endCursor } + } + } + } + }' -F owner="$owner" -F name="$name" -F number="$PR" \ + | jq -s '[ .[].data.repository.pullRequest.reviewThreads.nodes[] ]' > "$THREADS.raw" + + # line goes null once a thread is stale against the head commit, and + # originalLine still says where it was written -- which is what makes a + # thread on since-rewritten code readable rather than a finding at line 0. + # + # thread_id is the handle a review names to close one. The driver admits an id + # only when it matches a thread listed here, so this file is the allowlist as + # well as the history, and a thread left out of it can be named and not closed. + # + # The marker and the login reach jq through $ENV rather than through the shell, + # so the program stays one single-quoted string and no comment body is ever + # spliced into it. + # The root is the whole of the ownership test and the finding's own words; the + # replies are every recent comment that is not it, matched on id rather than on + # position, so a thread short enough to carry its own root in the recent window + # does not report the finding back as a reply to itself. + # + # reply_total is how many replies the thread has, against however many of them + # this carries. The count is written whether or not the two differ, so a reader + # of this file needs no second source to tell a whole conversation from a + # shortened one. Nothing consumes it yet: the driver renders "showing N of M" + # from it in the change that lands beside this one, and an older driver ignores + # a key it does not know. + # shellcheck disable=SC2016 # $ENV is jq's, and single quotes are what keep it jq's + jq '[ .[] + | select((.root.nodes[0].body // "") | startswith($ENV.FINDING_MARKER)) + | select(.root.nodes[0].author.login + | . != null and . != "" + and (. == $ENV.REVIEWER_LOGIN or . == $ENV.WORKFLOW_LOGIN)) + | . as $t + | { thread_id: (.id // ""), + file: (.path // ""), + line: (.line // .originalLine // 0), + body: ((.root.nodes[0].body // "") | sub($ENV.FINDING_MARKER + "\n*"; "")), + replies: [ $t.recent.nodes[] + | select(.id != $t.root.nodes[0].id) + | "\(.author.login // "someone"): \(.body)" ], + reply_total: (($t.recent.totalCount // 1) - 1), + resolved: .isResolved } ]' "$THREADS.raw" > "$THREADS" + fetched="$(jq length "$THREADS.raw")" + carried="$(jq length "$THREADS")" + echo "read $fetched review thread(s) across every page; carrying $carried prior finding(s) written by ${REVIEWER_LOGIN:-nobody this run could name} or ${WORKFLOW_LOGIN:-nobody} into this review" + + # A thread with more replies than one page of them is reported rather than + # quietly shortened. The newest are the ones kept, so what is missing is the + # middle of a long conversation -- worth knowing when a reply seems to answer + # something the history does not show. + # + # Counted over the threads this history actually carries, both tests applied. + # Counting every marked thread instead would report a shortened conversation on + # a run that carried no history at all, which says nothing true about what the + # review is working from. + # + # The window plus one, because totalCount counts the root and the root is + # fetched separately and carried in `body`. At totalCount 21 the recent window + # holds comments 2 to 21, which is every reply there is, and the history is + # whole; the first comment a reader actually loses appears at 22. + # shellcheck disable=SC2016 # $ENV is jq's + shortened="$(jq --argjson window 20 '[ .[] + | select((.root.nodes[0].body // "") | startswith($ENV.FINDING_MARKER)) + | select(.root.nodes[0].author.login + | . != null and . != "" + and (. == $ENV.REVIEWER_LOGIN or . == $ENV.WORKFLOW_LOGIN)) + | select(.recent.totalCount > $window + 1) + ] | length' "$THREADS.raw")" + if [ "$shortened" -gt 0 ]; then + echo "::warning::$shortened of this tool's thread(s) on $REPO#$PR carry more replies than this read holds; the 20 most recent of each are in the history and the older ones are not" + fi + + # A marked thread that the login test drops is worth a line, because the two + # ways to get here look identical from the outside: this reviewer has left + # nothing on the pull request yet, or it has and this run does not recognise + # its own identity. Only the second is a defect, and it costs the review its + # history and its ability to close a single thread. + # shellcheck disable=SC2016 # $ENV is jq's + marked="$(jq '[ .[] + | select((.root.nodes[0].body // "") | startswith($ENV.FINDING_MARKER)) + ] | length' "$THREADS.raw")" + if [ "$marked" -gt 0 ] && [ "$carried" -eq 0 ]; then + echo "::warning::$marked thread(s) on $REPO#$PR open with this tool's marker and none was written by ${REVIEWER_LOGIN:-nobody this run could name} or ${WORKFLOW_LOGIN:-nobody}, so this review carries no history and can close no thread" + fi + + - name: Record the commit under review + id: head + if: ${{ inputs.mode == 'review' && !cancelled() }} + # Tolerated, because this is a step that improves publishing and must not be + # able to prevent reviewing. Failing it hard would let a transient api error + # abort a review that would otherwise have run, since the drive step that + # follows carries no condition of its own. No publisher below reads the head + # for itself. Each one states what it cannot do when this step produces + # nothing. + continue-on-error: true + shell: bash + env: + GH_TOKEN: ${{ steps.identity.outputs.token || github.token }} + REPO: ${{ github.repository }} + PR: ${{ needs.guard.outputs.pr_number }} + run: | + set -euo pipefail + # Resolved once, here, and read from this output by every publisher below. + # Everything published later is published AGAINST a commit, and the review + # takes minutes. A publisher that reads the head for itself reads it after + # the review, so a push mid-review attaches this verdict -- and a green + # check -- to code the driver never saw. Several such reads also disagree + # with each other inside one run. + # + # This is the commit the driver is about to review, and the commit every + # publisher names. A push during the review moves the pull request's head. + # It does not move this. + sha="$(gh api "repos/$REPO/pulls/$PR" --jq .head.sha)" + echo "sha=$sha" >> "$GITHUB_OUTPUT" + echo "reviewing $REPO#$PR at $sha" + + - name: Read the nit setting from the pull request + id: nits + # The author of one pull request is who asks for a polish pass, so the ask + # is a label on that pull request rather than a setting in the caller. + # + # Its own step, and not part of the drive step below, because the read needs + # a GitHub token and that step's env reaches the driver process. Keeping the + # token here is what keeps the driver's credentials to omnigent alone. + # + # It costs one read of the pull request. The guard reads the same object for + # the skip label, under the App identity alone; this one has to answer a + # caller that configured no App as well, so it takes the identity above and + # its own call rather than a share of that one. + if: ${{ inputs.mode == 'review' && !cancelled() && inputs.nitpick-label != '' }} + # An opt-in for advice must not be able to stop a review. A step that fails + # leaves the output unset, the drive step reads that as off, and the review + # runs with nits off. + continue-on-error: true + shell: bash + env: + GH_TOKEN: ${{ steps.identity.outputs.token || github.token }} + REPO: ${{ github.repository }} + PR: ${{ needs.guard.outputs.pr_number }} + NITPICK_LABEL: ${{ inputs.nitpick-label }} + run: | + set -euo pipefail + # A label that is absent and a read that failed both leave nits off, so the + # warning below is the only thing that tells them apart in the log. It is + # where a polish pass that was asked for and did not happen is explained. + # + # Off, and not a refusal, because this label asks for advice rather than + # withholding work. The guard refuses on a signal it could not read, because + # admitting there runs a review it should not have run. Admitting here runs + # the review that was asked for, carrying one fewer kind of finding. + read_ok=true + # shellcheck disable=SC2016 # $ENV is jq's own, and jq reads it + carries="$(gh api "repos/$REPO/pulls/$PR" \ + --jq 'any(.labels[]?.name; . == $ENV.NITPICK_LABEL)')" || read_ok=false + if [ "$read_ok" = "false" ]; then + echo "::warning::could not read the labels on $REPO#$PR, so $NITPICK_LABEL cannot be found; this review runs without nits" + fi + if [ "$carries" != "true" ]; then carries=false; fi + echo "include_nits=$carries" >> "$GITHUB_OUTPUT" + echo "include nits on $REPO#$PR: $carries" + + - name: Read the base branch's accepted conditions + id: accepted + # The driver ENFORCES the Accepted list; it cannot fetch it. --guidelines-file + # names what the AGENT reads in its sandbox, which is guidance a model weighs. + # This is the copy this process hands the driver to decide whether a + # pre-existing blocker still withholds approval, so the fetch happens here. + # + # From the BASE ref, never the head. The head is this pull request's own merge, + # so a change that added an Accepted entry naming its own finding would clear + # the veto it caused. The driver records the ref beside every accepted finding + # for the same reason: a workflow that handed over the head's copy shows it on + # the pull request rather than hiding it. + # + # A repository with no standards file is the common case and not an error. Both + # outputs stay empty, the driver is passed neither flag, and nothing is + # accepted -- which is the same veto as before this step existed. + if: ${{ inputs.mode == 'review' && !cancelled() }} + continue-on-error: true + shell: bash + env: + GH_TOKEN: ${{ steps.identity.outputs.token || github.token }} + REPO: ${{ github.repository }} + PR: ${{ needs.guard.outputs.pr_number }} + GUIDELINES_FILE: ${{ inputs.guidelines-file }} + run: | + set -euo pipefail + # The driver's own DefaultGuidelinesFile, restated because this step fetches + # the file the driver enforces and the two have to name one file. + file="${GUIDELINES_FILE:-}" + if [ -z "$file" ]; then file="REVIEW.md"; fi + # This value reaches a URL, so anything but a plain repository path falls + # back. Character for character the driver's own isPlainRepoPath + # (internal/review/prompt.go): no leading slash, no parent reference, and + # nothing outside [A-Za-z0-9._/-]. The two have to agree -- a name this step + # accepts and the driver rejects reads one file and enforces another. + # + # One case statement, because the rule is one rule. Split across a case and + # a grep, one arm warned and the other fell back in silence, and the driver + # README tells an operator to check the logged path when standards seem not + # to apply. + case "$file" in + ""|/*|*..*|*[!A-Za-z0-9._/-]*) + echo "::warning::guidelines-file is not a plain repository path; reading REVIEW.md on $REPO instead" + file="REVIEW.md" ;; + esac + + base="$(gh pr view "$PR" --repo "$REPO" --json baseRefName --jq .baseRefName)" || base="" + if [ -z "$base" ]; then + echo "::warning::could not read the base branch of $REPO#$PR; nothing is accepted, so every pre-existing blocker withholds" + exit 0 + fi + + # Cleared before the write. RUNNER_TEMP survives between jobs on a + # non-ephemeral self-hosted runner, which is the steady state here, and + # between attempts on a re-run -- so a copy left by an earlier run is a file + # this one could publish as the base branch's. + out="$RUNNER_TEMP/base-standards.md" + rm -f "$out" + + # -f ref=, not ?ref=. A git ref may carry & # % or +, and one interpolated + # into the query string is read by GitHub as the end of the value: a wrong + # ref read, reported as the right one, which is the inverse of what + # --base-standards-ref exists to surface. gh puts -f on the query string for + # a GET and encodes it. + # + # stderr is captured rather than discarded, because an ABSENT file and an + # UNREADABLE one are the same outcome and not the same event. Both accept + # nothing, which is the safe direction and what the driver does. Only one of + # them is routine: a 403 or a rate limit silently re-imposes a veto the base + # branch had accepted, and a log line naming the file sends the operator + # after a file when the fault is the token. + # + # `2>&1 > "$out"`, and the order is the whole of it. stderr is duplicated to + # the substitution's pipe FIRST, then stdout is pointed at the file -- so the + # message reaches $err and the body reaches $out. Written the other way round + # both land in $out: the capture is empty, every failure reads as the generic + # one, and GitHub's error body is what this step would hand the driver as the + # base branch's standards. + # --method GET, and the read does not work without it. gh sends GET only + # while no parameter is supplied; `-f ref=` adds one and flips the request to + # POST, which puts the ref in a JSON body rather than the query string. There + # is no POST route for /contents, so every run took a 404 and reported it as + # "no standards file" -- the base branch's Accepted list never reached the + # driver, and every pre-existing blocker went on withholding approval on a + # repository that had accepted it. Same rule as the page size in the guard, + # and the same direction of failure: silent, and indistinguishable from the + # legitimate case. + if err="$(gh api --method GET "repos/$REPO/contents/$file" -f ref="$base" \ + -H "Accept: application/vnd.github.raw" 2>&1 > "$out")"; then + # A file that exists and is empty accepts nothing, and the drive step's own + # -s test would pass neither flag -- so taking the no-file branch here is + # what keeps the log from claiming a read that had no effect. + if [ -s "$out" ]; then + echo "path=$out" >> "$GITHUB_OUTPUT" + echo "ref=$base" >> "$GITHUB_OUTPUT" + echo "read $file from $base ($(wc -c < "$out") bytes)" + else + rm -f "$out" + echo "$REPO's $file is empty on $base; nothing is accepted" + fi + else + rm -f "$out" + case "$err" in + *"HTTP 404"*|*"Not Found"*) + echo "$REPO has no $file on $base; nothing is accepted" ;; + *) + echo "::warning::could not read $file from $base on $REPO: $err" + echo "::warning::nothing is accepted, so every pre-existing blocker withholds approval. This is a read failure and not an absent file -- check the token's contents: read scope and the API rate limit before adding the file" ;; + esac + fi + + - name: Drive session + collect verdict + id: drive + shell: bash + env: + BIN: ${{ steps.build.outputs.bin }} + OMNIGENT_BASE_URL: ${{ inputs.omnigent-base-url }} + SEIDROID_SCOUTS: ${{ inputs.scouts }} + SEIDROID_MODEL: ${{ inputs.claude-model }} + SEIDROID_ALLOW_TOOLS: ${{ inputs.allow-tools }} + MODE: ${{ inputs.mode }} + # The reviewed pull request. The driver keys the session on this pair. + REPO: ${{ github.repository }} + # The guard supplies this for a review; a close event carries its own + # number and skips the guard entirely. + PR: ${{ needs.guard.outputs.pr_number || github.event.pull_request.number }} + # trigger_id, not comment_id: this labels the dispatch in the driver's log + # and nothing else, so it wants the id whatever the event. comment_id is the + # reactable-object id and goes out empty where nothing can react on it, + # which would leave a review-body dispatch with no label at all. + TRIGGER_ID: ${{ needs.guard.outputs.trigger_id }} + # The findings this reviewer left before, so it drops what the author has + # addressed and keeps what the diff still shows. No token rides with it: + # this step reaches GitHub through nothing, which is the boundary that + # keeps the driver's credentials to omnigent alone. + THREADS: ${{ steps.threads.outputs.threads_path }} + GUIDELINES_FILE: ${{ inputs.guidelines-file }} + EXTRA_INSTRUCTIONS: ${{ inputs.extra-instructions }} + INCLUDE_NITS: ${{ steps.nits.outputs.include_nits }} + # The driver's own budget for this review. It bounds the step below and + # nothing else: the scout pass, the sandbox launches and the publish steps + # are outside it, which is why the job cap has to sit above it rather than + # at it. + SEIDROID_RUN_DEADLINE_S: ${{ inputs.run-deadline-seconds }} + # The Accepted list this process enforces, and the ref it came from. Empty + # where the repository keeps no standards file, or where the read failed. + BASE_STANDARDS: ${{ steps.accepted.outputs.path }} + BASE_STANDARDS_REF: ${{ steps.accepted.outputs.ref }} + run: | + set -euo pipefail + # OMNIGENT_MACHINE_CLIENT_ID/SECRET are already in the job env (see + # above) -- the driver mints its own bearer in-process from them and + # that token never transits a workflow step output. + # + # The verdict is written to a directory this run owns and clears first, + # never to the workspace. actions/checkout cleans .driver and nothing + # else, so a workspace-relative verdict.md left by an earlier run -- + # attempt 2 of this one, or any run on a non-ephemeral self-hosted + # runner, which is the steady state here -- satisfies the non-empty + # check below and gets posted as this review. + out_dir="$RUNNER_TEMP/review-out/$GITHUB_RUN_ID-$GITHUB_RUN_ATTEMPT" + rm -rf "$out_dir" + mkdir -p "$out_dir" + verdict="$out_dir/verdict.md" + echo "verdict_path=$verdict" >> "$GITHUB_OUTPUT" + # Written only when the review has observations that name a file and a + # line, so its absence means "summary only" rather than a failure. + findings="$out_dir/findings.json" + echo "findings_path=$findings" >> "$GITHUB_OUTPUT" + # Written whenever the review reached a verdict, including a clean one: + # a checks list with no review entry reads as a review that did not run. + check="$out_dir/check.json" + echo "check_path=$check" >> "$GITHUB_OUTPUT" + + if [ "$MODE" = "close" ]; then + args=(review "$REPO" "$PR" --close) + else + args=(review "$REPO" "$PR" --out "$verdict" --findings-out "$findings" + --check-out "$check") + # Absent when the read above failed, and the driver reads that as a + # first review rather than an error. + if [ -s "${THREADS:-}" ]; then args+=(--conversation-context "$THREADS"); fi + # Passed only when set, so the driver's own defaults stay the one + # place either is decided. + if [ -n "${GUIDELINES_FILE:-}" ]; then + args+=(--guidelines-file "$GUIDELINES_FILE") + fi + if [ -n "${EXTRA_INSTRUCTIONS:-}" ]; then + args+=(--extra-instructions "$EXTRA_INSTRUCTIONS") + fi + # Off means redirected rather than dropped: without this flag the driver + # sends a nit-grade finding to the non-blocking bucket, so it reaches the + # reader in prose and opens no thread on the line. + if [ "${INCLUDE_NITS:-}" = "true" ]; then args+=(--include-nits); fi + if [ -n "${TRIGGER_ID:-}" ]; then args+=(--trigger-id "$TRIGGER_ID"); fi + # The copy the DRIVER enforces. --guidelines-file above is the name the + # agent reads in its sandbox; this is the base branch's content, fetched + # out here because the driver cannot reach GitHub itself. The ref rides + # with it so a wrong one shows on the pull request instead of passing as + # the base. + if [ -s "${BASE_STANDARDS:-}" ]; then + args+=(--base-standards-file "$BASE_STANDARDS") + if [ -n "${BASE_STANDARDS_REF:-}" ]; then + args+=(--base-standards-ref "$BASE_STANDARDS_REF") + fi + fi + fi + set +e + "$BIN" "${args[@]}" + rc=$? + set -e + # Downstream gates on whether a verdict was PRODUCED (verdict.md + # non-empty), not on the exit code, so a review that reached one still + # publishes whatever else failed, and a no-verdict run never posts a + # placeholder. + if [ "$MODE" = "close" ]; then + # A close deletes a session; it has no verdict to produce, so the + # check below does not apply to it. Its own failure is the exit code, + # and it matters: this is the only thing that reclaims a sandbox, and + # nothing downstream notices when it does not. + echo "verdict_produced=false" >> "$GITHUB_OUTPUT" + if [ "$rc" -ne 0 ]; then + echo "::error::close failed (exit $rc) — the session and its sandbox are still running" + fi + elif [ -s "$verdict" ]; then + echo "verdict_produced=true" >> "$GITHUB_OUTPUT" + if [ "$rc" -ne 0 ]; then + echo "::warning::review exited $rc but produced a verdict (e.g. a teardown leak); see logs" + fi + else + echo "verdict_produced=false" >> "$GITHUB_OUTPUT" + echo "::error::review produced no verdict (exit $rc)" + # A review that produced nothing fails the job, whatever the driver + # exited. An annotation on a green job is a review nobody knows did not + # happen, and every step that reports one runs on !cancelled() rather + # than on success. 5 is the driver's own code for this. + if [ "$rc" -eq 0 ]; then rc=5; fi + fi + exit "$rc" + + - name: Place findings on the code + id: place + # Placement degrades in two steps rather than dropping a finding. The API + # accepts a line only where the diff covers it, and a review that reads the + # files around the diff -- which the prompt asks for, because a change can + # be locally correct and globally wrong -- cites both lines outside the + # hunks and files the pull request never touches. An uncovered line goes on + # its file; an untouched file has nowhere to go and is named in the summary. + # So the cost of a review that sees past the hunks is paid in placement, + # not in lost findings. + # + # Every finding the diff can hold rides in ONE review, which is what + # ai-review posts and what the author reads top to bottom on a single + # notification. Only the middle rung costs a call of its own: the reviews + # API carries a line comment and has no field for a file-level one. + # + # That review's body IS the verdict. One review carries the summary and the + # findings it is about, so the reader gets one notification and reads the + # verdict above the lines it cites, rather than a comment pointing at a review + # somewhere else on the page. The verdict step below then posts nothing, and + # falls back to a standalone comment only when this review does not land. + # + # It is COMMENTED, and that is what keeps it clear of the two selectors that + # look for this marker: both the guard's block check and the withdrawal below + # require CHANGES_REQUESTED as well, so neither can reach a review posted here. + # The position this run takes stays the position step's alone. + # + # Each comment's superseded thread ids are recorded as that comment posts, and + # the resolve step closes a thread only on finding its id in that record. So a + # review superseding three threads closes the ones whose own replacement reached + # the code and leaves the rest standing. A comment that degraded to the file rung + # counts as a replacement, for the reason that rung gives; the summary does not. + if: ${{ inputs.mode == 'review' && !cancelled() + && steps.drive.outputs.verdict_produced == 'true' }} + continue-on-error: true + shell: bash + env: + GH_TOKEN: ${{ steps.identity.outputs.token || github.token }} + REPO: ${{ github.repository }} + PR: ${{ needs.guard.outputs.pr_number }} + REVIEWED_SHA: ${{ steps.head.outputs.sha }} + FINDINGS: ${{ steps.drive.outputs.findings_path }} + # The verdict the driver rendered, which becomes the body of the review + # carrying the comments below. From v0.21.0 that body is composed from the + # structured block alone: summary first, then the line-less findings by + # severity, with the ones placed below counted in their section rather than + # repeated here. + VERDICT: ${{ steps.drive.outputs.verdict_path }} + # Findings that reached neither a line nor a file, collected for the + # summary. Declared here so the step below can read it by output. + NOTE: ${{ runner.temp }}/review-unplaced.md + # One superseded thread id per line, appended as the comment replacing it + # posts. Declared here so the resolve step can read it by output. + LINKAGE: ${{ runner.temp }}/review-superseded-placed.txt + run: | + set -euo pipefail + echo "note_path=$NOTE" >> "$GITHUB_OUTPUT" + : > "$NOTE" + # Emptied before anything is appended. RUNNER_TEMP survives a re-run on a + # non-ephemeral self-hosted runner, which is the steady state here, and a + # record left by attempt 1 would close a thread on attempt 2's behalf. + echo "linkage_path=$LINKAGE" >> "$GITHUB_OUTPUT" + : > "$LINKAGE" + if [ ! -s "$FINDINGS" ]; then + echo "no findings to place; the summary carries the review" + # Zero, and not silence. Nothing was placed because there was nothing to + # place, which is a number the summary can state. A step that was skipped + # writes nothing at all, and the summary tells the two apart by that. + # + # superseded_linked says whether the driver named which comment replaces + # which thread, and the resolve step picks its gate on it. Every path that + # returns before reading a finding says false, so the answer to a question + # this run never asked is the one that keeps a thread open. + { echo "on_line=0"; echo "on_file=0"; echo "unplaced=0" + echo "superseded_linked=false"; echo "summary_posted=false"; } >> "$GITHUB_OUTPUT" + exit 0 + fi + # The commit the review read, recorded before it started and used as + # recorded. A comment on any other commit points at code the review never + # saw, so the head is not read again here. + # + # Empty when that record failed. Every call below needs a commit id and the + # API rejects an empty one, so no comment can reach the diff and every + # finding takes the third rung of the ladder above: the summary. The reader + # loses the placement, not the finding. + head_sha="${REVIEWED_SHA:-}" + if [ -z "$head_sha" ]; then + echo "::warning::the reviewed commit was not recorded on $REPO#$PR; every finding goes to the summary instead of the diff" + fi + on_line=0 on_file=0 unplaced=0 + # Whether the verdict reached the pull request in the review below, and the + # review that carries it. The verdict step reads both: the first decides + # whether it posts a comment at all, the second is what it appends to. + summary_posted=false review_id="" + + # The third rung, in two groups. A finding reaches the summary because the + # diff has nowhere to attach it, or because the diff does carry its line and + # the call that would have posted it failed. The summary tells a reader which, + # so the second group is collected apart and headed for what it is. Both count + # as unplaced: neither reached the code. + # + # Each line opens with a dash and a backtick, which is what the summary counts + # when it cuts the list to fit GitHub's limit. + ondiff="$RUNNER_TEMP/review-unplaced-on-diff.md" + : > "$ondiff" + # shellcheck disable=SC2016 # the backticks are markdown, not a substitution + to_summary() { + printf -- '- `%s:%s` (%s) — %s\n' "$1" "$2" "$4" "$5" >> "$NOTE" + unplaced=$((unplaced+1)) + return 0 + } + # shellcheck disable=SC2016 # the backticks are markdown, not a substitution + to_summary_ondiff() { + printf -- '- `%s:%s` (%s) — %s\n' "$1" "$2" "$4" "$5" >> "$ondiff" + unplaced=$((unplaced+1)) + return 0 + } + + # The threads one comment replaced, recorded as that comment posts. The resolve + # step closes a superseded thread on finding its id here and on nothing else, + # so an id written for a comment that did not post takes a live finding off the + # pull request. Neither summary collector records: the summary is not the code. + # + # A dot is a finding that replaces nothing, and no node id can be one -- the + # alphabet GitHub mints from has no dot in it. The ids are space-separated and + # split back onto their own lines, because the record is read line by line. + record_superseded() { + [ "$1" = "." ] && return 0 + printf '%s\n' "$1" | tr ' ' '\n' >> "$LINKAGE" + return 0 + } + + # The second rung, one call per finding. A file-level comment is a field + # the reviews API does not carry, so this one cannot ride in the batch + # below. The file can still be in the pull request, and a comment on it + # reaches the reviewer in the file they are already reading, so the cited + # line rides in the body instead. + # + # Guarded on the commit, not left to the API. Without one the call returns + # 422, and asking for an answer known before the call spends the rate limit + # on it. + # + # $6 is which summary group takes the finding if this rung refuses it too. + # The caller knows whether the diff carries the line; this does not. + on_file_or_summary() { + if [ -n "$head_sha" ] && gh api -X POST "repos/$REPO/pulls/$PR/comments" \ + -f body="$FINDING_MARKER"$'\n'"**$4** — $5"$'\n\n'"_Cited at \`$1:$2\`, outside this diff's changed lines._" \ + -f commit_id="$head_sha" -f path="$1" \ + -f subject_type=file >/dev/null 2>&1; then + on_file=$((on_file+1)) + # A file comment replaces the thread. It is on the diff, in the file the + # reader is already in, and its body carries the cited line. The thread it + # replaces is one whose line this diff no longer covers -- which is why + # this rung was reached -- so not closing it leaves a duplicate for good. + record_superseded "$6" + return 0 + fi + "$7" "$1" "$2" "$3" "$4" "$5" "$6" + return 0 + } + + # All three rungs for one finding, with the API deciding which one it + # takes. This is the path for a finding the batch could not carry. + place_one() { + if [ -n "$head_sha" ] && gh api -X POST "repos/$REPO/pulls/$PR/comments" \ + -f body="$FINDING_MARKER"$'\n'"**$4** — $5" \ + -f commit_id="$head_sha" -f path="$1" \ + -F line="$2" -f side="$3" >/dev/null 2>&1; then + on_line=$((on_line+1)) + record_superseded "$6" + return 0 + fi + on_file_or_summary "$@" + return 0 + } + + # The detail is base64 per record, not @tsv. Finding.Detail is raw model + # prose with no line constraint on it, and @tsv escapes a newline, a tab or + # a backslash into a literal \n, \t or \\ -- so a multi-line detail reached + # the pull request showing its escape sequences instead of its text. The + # five fields that cannot contain a tab stay plain. + # + # The supersedes list rides ahead of the detail rather than after it, and the + # order the fields are READ in is not the order they are passed in. Only the + # last field may be empty; see the reader below. A finding replacing nothing + # writes a dot, so the list is never the empty field either. + tsv='[.file, .line, .side, .severity, + (.supersedes | if length == 0 then "." else join(" ") end), + (.detail | @base64)] | @tsv' + rows() { jq -r "$1 | $tsv" "$2"; } + # Redirected into, never piped into. The counters the summary reads are + # this shell's, and a pipe would count them in a subshell that exits. + place_each() { + local placer="$1" summary="$2" + local path line side severity supersedes detail_b64 detail + while IFS=$'\t' read -r path line side severity supersedes detail_b64; do + [ -z "$path" ] && continue + detail="$(printf '%s' "$detail_b64" | base64 --decode)" + "$placer" "$path" "$line" "$side" "$severity" "$detail" \ + "$supersedes" "$summary" + done + return 0 + } + + # One shape for every path below: a string file, an integer line, a side + # the API names. A line the model wrote as prose reads as 0, which no diff + # covers, so it takes the file rung rather than reaching gh as a field. + # + # Every field but the last is non-empty here, and that is what the reader + # below rests on. @tsv writes an empty field as nothing between two tabs, + # bash reads a tab as IFS whitespace and folds the pair into one delimiter, + # and the record then reads one field short: the severity arrives in the + # side, the base64 detail arrives in the severity, and a finding reaches the + # pull request with its own encoding printed as its severity. + # + # supersedes is the threads this finding's comment replaces. The driver admits + # every id against the threads it was handed, and this admits the SHAPE again: + # the record is read line by line and each id is matched whole, so an id + # carrying a space or a newline would split into two the resolve step then + # cannot find. Dropping one leaves its thread open, which is the safe end. + # A driver that publishes no linkage yields the empty list on every finding. + normalise='[ .[] | { file: (.file | tostring), + line: (.line | if type == "number" then floor + elif type == "string" then ((. | tonumber?) // 0 | floor) + else 0 end), + side: ((.side // "RIGHT") | tostring | ascii_upcase + | if . == "LEFT" then "LEFT" else "RIGHT" end), + severity: (.severity | tostring + | if . == "" then "note" else . end), + supersedes: [ (.supersedes // [])[] + | select(type == "string") + | select(test("^[A-Za-z0-9_=+/-]{1,200}$")) ], + detail: (.detail | tostring) } + | select(.file != "") ]' + normal="$RUNNER_TEMP/review-findings-normalised.json" + if ! jq "$normalise" "$FINDINGS" > "$normal"; then + echo "::warning::the findings file for $REPO#$PR could not be read, so this review places nothing on the diff" + { echo "on_line=0"; echo "on_file=0"; echo "unplaced=0" + echo "superseded_linked=false"; echo "summary_posted=false"; } >> "$GITHUB_OUTPUT" + exit 0 + fi + # One finding naming a thread is the whole test. An older driver publishes the + # key on none of them, and a review that supersedes nothing publishes it on + # none either -- and in that case there is no superseded thread for either gate + # to decide, so the two answers are the same one. + if jq -e 'any(.[]; (.supersedes | length) > 0)' "$normal" >/dev/null; then + echo "superseded_linked=true" >> "$GITHUB_OUTPUT" + else + echo "superseded_linked=false" >> "$GITHUB_OUTPUT" + fi + + # Which lines a comment can name, walked out of the diff the fetch below + # reads, before anything is posted. RIGHT takes an added or a context line, LEFT + # takes a removed or a context one, and ai-review walks the same patches the + # same way. The walk covers only the files the findings name, so its size is + # the finding count and not the diff's. + # + # Three groups, because a file the API sends without a patch -- a binary one, + # or one whose diff was too large to send -- has unknown lines rather than no + # lines. Counting it as no lines would tell an author their cited line is + # outside the diff when it is in it. Those findings stay out of the batch and + # go to the API one at a time, which is the only thing that can still say. + # + # Read ahead of the call because the call is all-or-nothing: one line the API + # refuses loses every comment with it, so a refusal has to be predicted here + # rather than met there. + # shellcheck disable=SC2016 # $findings and friends are jq's, and single quotes are what keep them jq's + partition='def hunks: + reduce ((.patch // "") | split("\n"))[] as $l + ({o: 0, n: 0, R: {}, L: {}}; + if ($l | test("^@@ -[0-9]+(,[0-9]+)? [+][0-9]+(,[0-9]+)? @@")) then + ($l | capture("^@@ -(?[0-9]+)(,[0-9]+)? [+](?[0-9]+)(,[0-9]+)? @@")) as $m + | .o = ($m.o | tonumber) | .n = ($m.n | tonumber) + elif ($l | startswith("+")) then .R[.n | tostring] = true | .n += 1 + elif ($l | startswith("-")) then .L[.o | tostring] = true | .o += 1 + elif ($l | startswith(" ")) then + .R[.n | tostring] = true | .L[.o | tostring] = true | .n += 1 | .o += 1 + else . end) + | {RIGHT: .R, LEFT: .L}; + (reduce $findings[0][] as $f ({}; .[$f.file] = true)) as $wanted + | ([ .[] | select($wanted[.filename // ""] // false) + | {key: .filename, + value: (if has("patch") then hunks else null end)} ] | from_entries) as $index + | ($findings[0] + | map(. as $g | ($index[$g.file]) as $h + | . + { ok: (($h[$g.side][$g.line | tostring]) // false), + unknown: (($index | has($g.file)) and ($h == null)) })) + | { anchored: [ .[] | select(.ok) | del(.ok, .unknown) ], + unknown: [ .[] | select(.ok | not) | select(.unknown) | del(.ok, .unknown) ], + loose: [ .[] | select(.ok | not) | select(.unknown | not) | del(.ok, .unknown) ] }' + files="$RUNNER_TEMP/review-pr-files.json" + placement="$RUNNER_TEMP/review-placement.json" + # The diff at the commit this review read, which is the commit every comment + # below names. GET /pulls/{n}/files answers for the pull request's current + # head and takes no commit: it accepts a sha parameter and ignores it, + # measured. So a push mid-review would index one commit and comment on + # another -- and a line marked from the newer diff that the older commit + # cannot carry costs the whole batch a 422. compare takes a commit. + # + # base.sha with three dots, so the API resolves the merge base itself and + # answers with the diff the pull request shows. A base branch that moves + # during the review does not move that merge base, because the reviewed + # commit is fixed and the fork point with it. + # + # The pull request itself, read once for two fields: the commit its diff + # starts from, and how many files that diff has. + # + # Both tested for shape, and not merely for emptiness. A read that fails + # leaves the API's error object in the file, and a field that object does + # not carry reads as null -- which would otherwise reach the API as a + # commit id. + is_count() { case "${1:-}" in (''|*[!0-9]*) return 1 ;; esac; } + pull="$RUNNER_TEMP/review-pull.json" + base_sha="" changed_files="" + if [ -n "$head_sha" ]; then + if gh api "repos/$REPO/pulls/$PR" > "$pull" 2>/dev/null; then + base_sha="$(jq -r '.base.sha // "" | tostring' "$pull" 2>/dev/null || true)" + changed_files="$(jq -r '.changed_files // "" | tostring' "$pull" 2>/dev/null || true)" + fi + case "$base_sha" in (''|*[!0-9a-f]*) base_sha="" ;; esac + is_count "$changed_files" || changed_files="" + if [ -z "$base_sha" ]; then + echo "::warning::the base commit of $REPO#$PR could not be read, so the diff at $head_sha cannot be; each finding is posted on its own" + fi + fi + # compare sends at most 300 files and drops the rest without saying so: no + # total, no Link header for them, no flag. Its pages are pages of commits, + # and a second page carries no files key at all, so there is nothing to page + # for. Measured: a 398-file pull request answers with 300. + # + # A short list cannot be told apart from a whole one, and a file missing + # from it looks exactly like a file the pull request never touched. Reading + # it as the diff would put a finding on its file under a body telling the + # author their cited line is outside a diff that holds it. So the list is + # counted against the pull request's own total, and a short one indexes + # nothing: every finding then goes to the API one at a time, which is the + # only thing that can still say where it belongs. + # + # 300 is that cap, so a list of 300 is at it. That test only has to carry + # the case where the total could not be read, because the total is exact. + build_index() { + local n + if gh api "repos/$REPO/compare/$base_sha...$head_sha?per_page=100" > "$files.raw" \ + && jq '[ (.files // [])[] ]' "$files.raw" > "$files" \ + && n="$(jq 'length' "$files")" && is_count "$n"; then + if [ -n "$changed_files" ] && [ "$n" -ne "$changed_files" ]; then + echo "::warning::the diff of $REPO#$PR at $head_sha came back with $n of its $changed_files file(s), so this run does not read it as the diff; each finding is posted on its own" + return 1 + fi + if [ -z "$changed_files" ] && [ "$n" -ge 300 ]; then + echo "::warning::the diff of $REPO#$PR at $head_sha came back with $n file(s), which is all this endpoint sends, and the pull request's own total could not be read; each finding is posted on its own" + return 1 + fi + jq --slurpfile findings "$normal" "$partition" "$files" > "$placement" && return 0 + fi + echo "::warning::the diff of $REPO#$PR at $head_sha could not be read, so each finding is posted on its own; the review still reaches the code" + return 1 + } + batched=false + if [ -n "$head_sha" ] && [ -n "$base_sha" ] && build_index; then + batched=true + fi + + if [ "$batched" = true ]; then + anchored="$(jq '.anchored | length' "$placement")" + if [ "$anchored" -gt 0 ]; then + # The marker opens every comment body, as the first bytes. The history + # read and the resolve step both recognise this tool's own findings by + # that, and a body that merely contains it matches neither. + request="$RUNNER_TEMP/review-placement-request.json" + response="$RUNNER_TEMP/review-placement-response.json" + # The body is the verdict, under the marker that names this tool's own. + # A driver at the floor renders it summary-first and counts these + # comments in their section rather than repeating them, so the review + # reads as one piece: what the change needs, then the lines it needs it on. + # + # The fallback body is not a second verdict. If the driver wrote none to + # carry, this review still has to say something, and what it says is where + # the verdict is -- which is then the standalone comment the verdict step + # posts, because summary_posted stays false on this path. + carries_verdict=false + if [ -n "${VERDICT:-}" ] && [ -s "$VERDICT" ]; then + review_body="$VERDICT_MARKER"$'\n'"$(cat "$VERDICT")" + carries_verdict=true + else + review_body="Findings on the changed lines. The verdict and the summary are in this tool's comment on this pull request." + echo "::warning::the driver left no verdict for the review on $REPO#$PR to carry, so it names where one will be and the verdict is posted on its own" + fi + # shellcheck disable=SC2016 # $sha and friends are jq's, bound by --arg + jq --arg sha "$head_sha" --arg marker "$FINDING_MARKER" --arg body "$review_body" \ + '{commit_id: $sha, event: "COMMENT", body: $body, + comments: [ .anchored[] + | {path: .file, line: .line, side: .side, + body: "\($marker)\n**\(.severity)** — \(.detail)"} ]}' \ + "$placement" > "$request" + # Two failures, two answers. A 4xx is the API refusing the request and + # it creates nothing, so each finding is posted on its own and the one + # comment that was refused costs only itself. This request carries every + # finding's whole detail, which is model prose under no length bound, so + # the size of it is refused as readily as the content: 413 and 422 are + # the same answer here, and so is a 403 the retry can still get past one + # comment at a time. + # + # Any other failure may be a write that landed and then lost its + # connection, and repeating it posts the review twice -- so those + # findings take the summary, where the reader still gets every one of + # them. + # + # The code comes from the response's own status line, which -i puts + # first, and not from a status field in the error body. GitHub's + # validation-error schema declares no such field, and a refusal that + # carries none would read as no refusal at all and send every anchorable + # finding to the summary -- worse than placing each one by hand, which + # is what this rung is for. + # + # A call that reached no response leaves no status line, and an empty + # code takes the summary. That is the case where the write may have + # landed, so it is the case that must not be repeated. + if gh api -i -X POST "repos/$REPO/pulls/$PR/reviews" --input "$request" > "$response"; then + on_line=$((on_line + anchored)) + # The call carries every anchored comment and creates all of them or + # none, so one write records the whole batch's linkage. + jq -r '.anchored[] | .supersedes[]' "$placement" >> "$LINKAGE" + # Both answers describe the review that carries the VERDICT, so neither + # is written for a review that carries only the findings. A run whose + # driver wrote no verdict leaves them as they started, and the verdict + # step posts the verdict on its own. + # + # The verdict is on the pull request the moment this call returns, so + # that step must not post a second copy of it. That holds even where the + # id could not be read: what an unreadable id costs is the append, and + # what would have been appended goes to the log instead. + if [ "$carries_verdict" = true ]; then + summary_posted=true + # -i puts the status line and the headers ahead of the body, so the + # JSON begins after the first empty line. A value that is not a number + # is no id, and reaching the API with one would edit nothing and report + # that it had. + review_id="$(sed '1,/^[[:space:]]*$/d' "$response" | jq -r '.id // empty' 2>/dev/null || true)" + case "$review_id" in (''|*[!0-9]*) review_id="" ;; esac + if [ -z "$review_id" ]; then + echo "::warning::the review carrying the verdict posted on $REPO#$PR but its id could not be read, so what the verdict step would append to it goes to this run's log" + fi + fi + echo "posted one review carrying $anchored comment(s) on $REPO#$PR" + else + status="$(sed -n '1s|^HTTP/[0-9.]* \([0-9][0-9][0-9]\).*|\1|p' "$response" || true)" + case "$status" in + 4??) + echo "::warning::$REPO#$PR refused the review carrying $anchored comment(s) with $status; each finding is posted on its own instead" + place_each place_one to_summary_ondiff < <(rows '.anchored[]' "$placement") ;; + *) + echo "::warning::the review carrying $anchored comment(s) could not be posted on $REPO#$PR; those findings are in the summary instead of on the diff" + place_each to_summary_ondiff '' < <(rows '.anchored[]' "$placement") ;; + esac + fi + fi + place_each place_one to_summary < <(rows '.unknown[]' "$placement") + place_each on_file_or_summary to_summary < <(rows '.loose[]' "$placement") + else + place_each place_one to_summary < <(rows '.[]' "$normal") + fi + # A heading per reason, because the summary collects findings for three and + # only one of them is about code the reader did not change. With no commit to + # attach to, a finding on a changed line lands here as well; so does one whose + # review the API would not take. Filing either under "off the changed lines" + # tells the author the wrong thing about their own diff, and the second is the + # one that hides a real issue behind a heading that denies it. + if [ -s "$ondiff" ] || [ -s "$NOTE" ]; then + if [ -n "$head_sha" ]; then + header='**Observations off the changed lines.** These are about code this pull request does not touch, so there is nowhere in the diff to attach them:' + else + header='**Every finding is here.** The commit under review was not recorded, so none of these could be attached to a line of the diff:' + fi + { printf -- '---\n' + if [ -s "$ondiff" ]; then + printf -- '\n%s\n\n' '**On the changed lines, and not posted.** GitHub would not take the review carrying these, so they are here rather than on the lines they name:' + cat "$ondiff" + fi + if [ -s "$NOTE" ]; then + printf -- '\n%s\n\n' "$header" + cat "$NOTE" + fi + } > "$NOTE.tmp" + mv "$NOTE.tmp" "$NOTE" + fi + # Here and at the two early exits above, and nowhere between: the points + # where these are final. A run that dies in between leaves them unwritten, + # which is the right answer there -- placement neither finished nor was + # skipped, so no number it could publish would be true. + # + # One append for all three, so the summary never reads a half-written set. It + # requires all three for that reason, including the one no term renders. + { echo "on_line=$on_line"; echo "on_file=$on_file"; echo "unplaced=$unplaced" + echo "summary_posted=$summary_posted"; echo "review_id=$review_id"; } >> "$GITHUB_OUTPUT" + echo "findings: $on_line on a line, $on_file on a file, $unplaced in the summary" + echo "verdict carried by the review: $summary_posted" + + - name: Publish the review check run + # The half of a review a reader sees without opening it. `review` is the check + # of record. The same verdict goes out under `AI Review` too, for the reason + # the second publish below states. + # + # Whichever check the driver wrote, decided or not. A run that reached no + # verdict writes one under the title `no verdict`, naming why, and publishing + # that is the point of dropping the verdict gate here: with nothing in the + # checks list, a review that ran and could not be read is indistinguishable + # from one that never ran, and only one of the two is a reason to look. The + # file's presence is the gate, so a run that never reached the driver still + # publishes nothing. + # + # That check concludes `failure`, so a repository requiring it holds the merge + # until a run reads the change. Read the title, not the conclusion, to tell the + # two apart: a decided review carrying blockers concludes `failure` too. + # + # Tolerated while there is a commit to publish against, like the other + # publish steps: a check run that fails to post must not bury a review that + # was produced, and the steps below run on !cancelled() and still publish it. + # + # Not tolerated when the reviewed commit is missing. head_sha is required and + # has no default, so that is not a post that failed and may work next time -- + # it is a merge gate that cannot exist, and a green job hides it. + if: ${{ inputs.mode == 'review' && !cancelled() }} + continue-on-error: ${{ steps.head.outputs.sha != '' }} + shell: bash + env: + GH_TOKEN: ${{ steps.identity.outputs.token || github.token }} + REPO: ${{ github.repository }} + PR: ${{ needs.guard.outputs.pr_number }} + REVIEWED_SHA: ${{ steps.head.outputs.sha }} + CHECK: ${{ steps.drive.outputs.check_path }} + AI_REVIEW_COPY: ${{ inputs.publish-ai-review-check }} + # The one field this step will not read out of the artifact. See below. + VERDICT_PRODUCED: ${{ steps.drive.outputs.verdict_produced }} + run: | + set -euo pipefail + if [ ! -s "$CHECK" ]; then + # Nothing to publish is not the same as nothing to say. Check runs are keyed + # on the commit and GitHub surfaces the latest per name, so on a commit an + # earlier review already passed, a re-review that dies before the driver -- + # a failed install, the MIN_DRIVER_VERSION refusal -- leaves that success + # standing as the current `review`. A branch rule requiring it then reads + # green for a review that did not happen, while the no-verdict notice on the + # pull request says otherwise. The gate and the notice must not disagree. + # + # Same treatment `Post the verdict`'s own failure path gives the same + # hazard, under the same names, so neither leaves a stale conclusion. + # + # With no commit there is no gate to correct, and none to be wrong either. + if [ -z "${REVIEWED_SHA:-}" ]; then + echo "no check run to publish, and no commit to publish one against" + exit 0 + fi + echo "no check file; publishing failure so an earlier run's conclusion on $REVIEWED_SHA does not stand for this one" + incomplete() { + gh api -X POST "repos/$REPO/check-runs" \ + -f name="$1" \ + -f head_sha="$REVIEWED_SHA" \ + -f status=completed \ + -f conclusion=failure \ + -f output[title]="review did not complete" \ + -f output[summary]="This review ended before the driver wrote a result, so it reached no conclusion about this commit. Read the workflow run: $GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" \ + >/dev/null + } + incomplete review \ + || echo "::warning::the incomplete-review check run could not be posted on $REPO#$PR; an earlier run's conclusion may still stand on $REVIEWED_SHA" + if [ "${AI_REVIEW_COPY:-}" = "true" ]; then + incomplete "AI Review" \ + || echo "::warning::the AI Review copy of the incomplete-review check run could not be posted on $REPO#$PR" + fi + exit 0 + fi + # A run that produced no verdict publishes failure, whatever the file says. + # The file is written by a separate process across a repository boundary, so + # reading a passing conclusion out of it on a run this workflow already knows + # produced nothing would let one crash between two writes satisfy a required + # check. This step decides the one thing that gates a merge; it reads the + # artifact for everything else. + conclusion="$(jq -r '.conclusion // empty' "$CHECK" || true)" + if [ "${VERDICT_PRODUCED:-}" != "true" ]; then + conclusion="failure" + fi + if [ -z "$conclusion" ]; then + echo "::error::the check file on $REPO#$PR carries no conclusion, so there is nothing to publish as the merge gate" + exit 1 + fi + # The same default again. A jq that fails leaves the title empty, and the `//` + # above only covers a field that is absent. The summary needs no such line, + # because its default is the empty string either way. + title="$(jq -r '.title // "review"' "$CHECK" || true)" + title="${title:-review}" + summary="$(jq -r '.summary // ""' "$CHECK" || true)" + # Against the commit the review read, recorded before it started and used + # as recorded. A check on any other commit attaches this verdict to code the + # review never saw, so the head is not read again here. + # + # Empty when that record failed. There is no weaker check run to publish in + # its place, so this says what is missing and fails; the step comment above + # says why that reaches the job. + head_sha="${REVIEWED_SHA:-}" + if [ -z "$head_sha" ]; then + echo "::error::the reviewed commit was not recorded on $REPO#$PR, so the review check run cannot be published; the verdict comment is the only record of this review" + exit 1 + fi + publish() { + gh api -X POST "repos/$REPO/check-runs" \ + -f name="$1" \ + -f head_sha="$head_sha" \ + -f status=completed \ + -f conclusion="$conclusion" \ + -f output[title]="$title" \ + -f output[summary]="$summary" >/dev/null + } + # The check of record, and the only publish here that can fail the step. A + # failure exits it under set -e, and continue-on-error above decides what + # that costs the job. + publish review + names="check run review" + # The same verdict under the name ai-review.yml publishes. A branch-protection + # rule matches a check by its name, so only a check called `AI Review` + # satisfies a rule that requires `AI Review`. The rules on the calling + # repositories are org-level, and no token here can read them. This step + # therefore publishes both names, and a rule that requires either one passes. + # Delete this call once someone who can read those rules confirms that none + # names `AI Review`. + # + # Best-effort, and that is the whole difference from the publish above. A + # failure here warns and the step still succeeds, so the copy never decides + # whether the review reached the pull request. + # + # A repository that also runs ai-review.yml under this App gets two checks + # called `AI Review` on the commit. GitHub keeps both and lists one: the one + # that completed last. The two publishes therefore race for the name, and + # `publish-ai-review-check` is how such a repository stands this copy down. + if [ "${AI_REVIEW_COPY:-}" = "true" ]; then + if publish "AI Review"; then + names="check runs review and AI Review" + else + echo "::warning::the AI Review copy of the review check could not be published on $REPO#$PR; review carries the verdict" + fi + fi + echo "published $names: $conclusion — $title" + + - name: State the review's position on the pull request + # The check run is the gate a merge reads; this is the one a person reads, + # and the one that shows in the reviewers list. Both come from the same + # conclusion the driver derived from the findings. + # + # Only when there is a position to take. A clean run with approval off, or + # a run that concluded neutral, adds nothing a reader does not already have + # from the comment above — and a review cannot be edited later the way that + # comment is upserted, so an opinionless one is permanent clutter. + # + # A review that reached no verdict has no position at all, and verdict_produced + # is what holds that. It is load-bearing twice over: the withdrawal at the end + # of this step reads the driver's finding counts, the no-verdict check carries + # none, and the no-counts branch there proceeds by design. So an ungated + # no-verdict run would clear a standing block on the strength of a review that + # produced nothing. The check run and the comment publish without this gate; + # anything that clears a merge gate keeps it. + # + # No continue-on-error, for the withdrawal at the end. That is the only work + # in this job that clears a merge gate, and a gate left standing on a green + # run is a block nobody knows to look for. Every other failure here is caught + # at its own site and stated there, so an unreadable check file, a position + # the API refuses and a courtesy log line all stay quiet. What reaches the job + # is the withdrawal. + # + # The App records the position where the caller configured one, and the + # workflow's own identity records it otherwise, so a repository with no App + # still takes a position and can still clear a block it left before. That + # identity reads as github-actions[bot], which costs two things a caller has + # to know: a merge gate that names its reviewers may not count it, and a + # repository whose Actions policy forbids an approval refuses the event + # outright. The COMMENT rung below answers the second. + if: ${{ inputs.mode == 'review' && !cancelled() + && steps.drive.outputs.verdict_produced == 'true' }} + shell: bash + env: + GH_TOKEN: ${{ steps.identity.outputs.token || github.token }} + REPO: ${{ github.repository }} + PR: ${{ needs.guard.outputs.pr_number }} + REVIEWED_SHA: ${{ steps.head.outputs.sha }} + CHECK: ${{ steps.drive.outputs.check_path }} + APPROVE_ON_SUCCESS: ${{ inputs.approve-on-success }} + run: | + set -euo pipefail + if [ ! -s "$CHECK" ]; then + echo "no conclusion to take a position from" + exit 0 + fi + # jq's error goes to the log and the value comes back on stdout, so a check + # file this step cannot read states itself rather than arriving as data. + # + # The RECORDED decision, not a position this step re-derives from the + # conclusion. The driver decides both in one reading: `approve` only where + # nothing withholds -- the reply affirmed it read the diff, nothing blocks, + # and no pre-existing blocker stands unaccepted. + # + # Re-deriving here is what shipped a comment ending "Approving", a footer + # reading decision `approve`, a neutral check and NO REVIEW AT ALL, on three + # pull requests, with nothing on any of them saying why. Two computations of + # one question will disagree; this step no longer holds the second. + # + # `// empty` rather than a bare field. A missing value renders as the string + # "null", which every comparison below would take for a decision. + decision="$(jq -r '.decision // empty' "$CHECK" || true)" + if [ -z "$decision" ]; then + # Warned, not exited. This step does two things and they fail apart: a + # missing decision costs the POSITION, and taking the withdrawal with it + # would strand a block an earlier run left on a finding this run did not + # reproduce -- which only a human can then clear. + echo "::warning::the check file names no decision, so no position is recorded on $REPO#$PR; the withdrawal below still runs, on the conclusion" + fi + # The conclusion is still read, for the WITHDRAWAL at the end of this step and + # for the log lines that name it. The two answer different questions and the + # split is the point: `decision` is the position this run records on the pull + # request, and `conclusion` plus the counts decide whether this run has the + # standing to clear a block an EARLIER one left. Nothing below re-derives an + # event from it. + conclusion="$(jq -r '.conclusion // empty' "$CHECK" || true)" + if [ -z "$conclusion" ]; then + # The withdrawal is the one thing in this job that clears a merge gate, and + # it reads the conclusion to decide whether this run has the standing to. + # A sentinel that matched none of its branches would fall THROUGH it and + # dismiss the block anyway -- on a check file this step could not read. + # `unknown` is named in the guard below for that reason. + echo "::warning::the check file names no conclusion, so no block is withdrawn on $REPO#$PR; an earlier one stands until a run that reads one" + conclusion="unknown" + fi + + # The commit the review read, recorded before it started and used as + # recorded. The head is not read again here, or in any other publisher: one + # read is the state this whole run publishes against. + # + # Empty only when that record failed. commit_id is optional on the reviews + # API and defaults to the pull request's latest commit, so the position + # still lands -- the weaker guarantee, stated rather than assumed. This is + # the one publisher that has a weaker version to fall back to. + head_sha="${REVIEWED_SHA:-}" + if [ -z "$head_sha" ]; then + echo "::warning::the reviewed commit was not recorded; the position goes on the current head of $REPO#$PR, which the review may not have read" + fi + + # Whether the pull request moved while the review ran. The driver resolves + # the head itself and takes no commit from this file, so the recorded sha + # binds the PUBLISHERS and not the read: a push landing mid-review means + # every publisher names commit A while the model read commit B. A push does + # not cancel this run, so nothing corrects it afterwards. + # + # THREE answers, not two: `same`, `moved`, and `unknown` -- because the two + # readers below want opposite things from the third one, and collapsing it + # into `moved=false` gave them both the approval's answer. + # + # `unknown` is a commit that was never recorded, or a read that failed or came + # back as something that is not a sha. It is not `same`. The approval treats + # it as one, deliberately: the window is seconds, and refusing an approval on + # an unanswered question would cost the common case to guard the rare one. + # + # The WITHDRAWAL cannot. It is the one publisher whose mistake does not heal: + # a block left standing is cleared by the next review, and a block cleared on + # a head nobody confirmed needs a human to put back. So it requires `same` and + # treats `unknown` as `moved`. That asymmetry is the whole reason this is three + # states rather than a boolean. + head_state=unknown + if [ -n "$head_sha" ]; then + now="$(gh api "repos/$REPO/pulls/$PR" --jq '.head.sha // empty' 2>/dev/null || true)" + case "$now" in (''|*[!0-9a-f]*) now="" ;; esac + if [ -n "$now" ]; then + if [ "$now" = "$head_sha" ]; then + head_state=same + else + head_state=moved + echo "::warning::$REPO#$PR moved from $head_sha to $now while this review ran; what it publishes names the commit it read, so it does not approve and does not clear an earlier block" + fi + fi + fi + + event="" + note="" + case "$decision" in + request_changes) + event=REQUEST_CHANGES + note="review found something blocking. The findings are on the lines they are about, and the summary is in this tool's verdict on this pull request." + ;; + approve) + # approve-on-success stays the caller's policy, and it is the only thing + # this step still decides. Where it is off, the review states its + # position in the comment and records none here. + # + # An approval also needs the commit. The fallback above -- let the API + # default to the pull request's current head -- is the weaker guarantee + # for a comment and for a block, and both stay true of code nobody read: + # a comment says something, a block withholds something. An approval + # ASSERTS something about a specific diff, so the same fallback would + # approve a commit pushed during the review, and under approve-on-success + # with an approval-count rule that approval is the gate. `Publish the + # review check run` refuses to exist without the commit for this reason; + # an approval is the same kind of claim. + # + # Downgraded rather than dropped: the review still records that it found + # nothing blocking, and the position it does not take is the one it + # cannot stand behind. + if [ "$APPROVE_ON_SUCCESS" = "true" ]; then + if [ -n "$head_sha" ] && [ "$head_state" != moved ]; then + event=APPROVE + note="review found nothing blocking." + elif [ -z "$head_sha" ]; then + event=COMMENT + note="review found nothing blocking. It does not approve: the commit it read was not recorded, so an approval here would name whatever is at the head of this pull request now." + echo "::warning::the reviewed commit was not recorded on $REPO#$PR, so this review comments where it would have approved; an approval would have named a commit the review may not have read" + else + event=COMMENT + note="review found nothing blocking in the commit it read. It does not approve: this pull request was pushed to while the review ran, so the approval would stand against code the review never saw. Ask for another review to have the new commit read." + fi + fi + ;; + comment) + # Only where something actually WITHHELD -- an unaccepted pre-existing + # blocker, a reply that never affirmed it read the diff, or a reply that + # wrote nothing down. Those are exactly the states the driver concludes + # something other than success for, and they are the states where its + # own notice names the finding that decided it. + # + # `comment` beside `success` is a different thing: the agent wrote the + # word, nothing withheld, and the driver publishes NO notice. Recording + # a review there would claim a finding that does not exist, beside a + # thumbs-up the reaction step leaves off the same success. So this takes + # no position, which is what it did before v0.20.0. + if [ "$conclusion" != "success" ]; then + event=COMMENT + note="review took no position. Its verdict on this pull request names the finding that withheld approval." + fi + ;; + "") + # Already warned above, where the field was read. A second line here + # would report one missing field twice. + ;; + *) + echo "::warning::unrecognised decision \"$decision\" on $REPO#$PR; no position taken" + ;; + esac + + # commit_id is omitted rather than sent empty when the commit is unknown: + # the API rejects an empty one, and its own default is the pull request's + # latest commit, which is the fallback announced above. + # + # 0 posted, 2 refused, 1 anything else. Only a refusal earns the rung below: + # a 422 is the API declining the opinion itself, and every other failure is + # one a second call would meet again. That keeps a write that landed and then + # lost its connection from posting a second review. + # + # 422, and not 403. GitHub answers a self-review with 422 -- measured, as + # "Review Can not approve your own pull request" -- and answers a secondary + # rate limit with 403 or 429. This endpoint creates content and notifies, so + # it is one the API names as tripping that limit. A rung that fired on 403 + # would turn a rate-limited REQUEST_CHANGES into a comment that blocks + # nothing, where one wait would have kept the block. The narrow rule costs + # the opposite case: a refusal that arrives as 403, such as an Actions policy + # that forbids an approval, earns no comment review and only the warning + # below. Losing the vote is the worse half, so the rule protects it. + # + # A 422 also covers a stale commit_id, which the rung retries with the same + # commit_id and meets identically. That costs one call and no review. + # + # The code comes from GitHub's error object, not from gh's message. gh writes + # the response to stdout and its own line to stderr, so a failed call leaves + # the error object in `body` -- and `status` on it is the code. gh's line is + # a human string outside its compatibility surface, and a reformat there + # would turn this rung into a silent no-op. + # + # `status` is documented on GitHub's Basic Error schema and measured on this + # endpoint's 422, but the 422's own schema does not declare it. So it can be + # absent, and an absent code returns 1: no rung, no downgrade. The vote + # survives a shape this cannot read, which is the same direction the 422 rule + # chooses above. + # + # gh's own line still reaches the log, where it says why. + post_position() { + local args body status + args=(-X POST "repos/$REPO/pulls/$PR/reviews" + -f event="$1" -f body="$VERDICT_MARKER"$'\n'"$note") + if [ -n "$head_sha" ]; then + args+=(-f commit_id="$head_sha") + fi + body="$(gh api "${args[@]}")" && return 0 + status="$(printf '%s' "$body" | jq -r '.status // empty' 2>/dev/null || true)" + if [ "$status" = "422" ]; then + return 2 + fi + return 1 + } + + if [ -n "$event" ]; then + # Guarded, because set -e would abort the step here and take the + # withdrawal below with it. A 422 on self-approval, a stale commit_id or + # a transient 5xx must not also cost the pull request its retraction. The + # API's own error stays on stderr, where it says why. + # + # COMMENT is the rung under both events, and the same one ai-review takes. + # The API refuses an opinion the identity may not hold -- github-actions + # approving where the repository's Actions policy forbids it, or either + # identity taking a side on a pull request it opened itself -- and refuses + # nothing about a review that states its finding without voting. So the + # reviewers list still carries this run, and the check run still carries + # the gate. What the rung costs is the vote: an APPROVE that lands here + # clears no approval requirement, and a REQUEST_CHANGES blocks nothing. + rc=0 + post_position "$event" || rc=$? + if [ "$rc" -eq 0 ]; then + echo "recorded $event on $REPO#$PR" + elif [ "$rc" -eq 2 ] && [ "$event" != "COMMENT" ] && post_position COMMENT; then + echo "::warning::$REPO#$PR would not take $event, so this review is recorded as a comment; the check run carries the $conclusion conclusion" + else + echo "::warning::could not record $event on $REPO#$PR; the verdict comment stands" + fi + else + echo "no position to record for a $conclusion conclusion" + fi + + # An earlier block this run no longer stands behind. Left standing it + # keeps a pull request red for a finding that is gone, and only a human + # can clear it. + # + # Withdrawn on the two things this run has to be able to say: it read the + # change, and it found nothing blocking in it. A failure says the second is + # false and stands behind its own block. + # + # The event is named as well, and it has to be. Deriving the event from the + # conclusion USED to make "posted REQUEST_CHANGES" and "concluded failure" + # one fact, so the failure test covered both. Reading the decision from the + # driver split them, and without this term a run can record a block and + # dismiss it in the same step -- its own, seconds old, on the strength of a + # conclusion that disagrees with the position it just took. + # + # `unknown` is the sentinel from the unreadable-conclusion branch above. It + # matches nothing else here, so naming it is what stops an unreadable check + # file from clearing a merge gate. + if [ "$event" = "REQUEST_CHANGES" ] || [ "$conclusion" = "failure" ] \ + || [ "$conclusion" = "unknown" ]; then + exit 0 + fi + # success says both, because the driver gates it on the line count the review + # reported. neutral says one word over three states -- a review that could not + # show it read the diff, a review whose only blocker is already on the base + # branch, and a review whose findings are all non-blocking. Only the first has + # no ground to clear another review's finding, and the counts are what separate + # it: something written down is a review of the change, and nothing written down + # beside a soft conclusion is a review that did not happen. + # + # Read under `counts`, where the driver writes them, with a sentinel for the + # field it does not: a bare null is a value the arithmetic below would take for + # a zero, and a driver that reports nothing would then read as a review that + # found nothing. + is_count() { case "${1:-}" in (''|*[!0-9]*) return 1 ;; esac; } + blocking="" non_blocking="" pre_existing="" counted=false + read -r blocking non_blocking pre_existing < <(jq -r '[(.counts.blocking // "?"), + (.counts.non_blocking // "?"), (.counts.pre_existing // "?")] | @tsv' \ + "$CHECK" 2>/dev/null) || true + if is_count "$blocking" && is_count "$non_blocking" && is_count "$pre_existing"; then + counted=true + fi + + # A blocking finding beside a conclusion that does not fail is the driver + # disagreeing with itself, and the comment step states it as that. Here it + # stops the withdrawal: whichever half is wrong, clearing a merge gate while + # the review names a blocker is the one outcome that cannot be walked back. + if [ "$counted" = true ] && [ "$blocking" -gt 0 ]; then + echo "::warning::the review reports $blocking blocking finding(s) beside a $conclusion conclusion, so it does not clear an earlier block on $REPO#$PR" + exit 0 + fi + if [ "$conclusion" != "success" ] && [ "$counted" = true ] \ + && [ $(( blocking + non_blocking + pre_existing )) -eq 0 ]; then + echo "::notice::a $conclusion review that wrote nothing down does not clear an earlier block on $REPO#$PR" + exit 0 + fi + # A dismissal asserts the current head is clean, so it needs the head + # CONFIRMED -- not merely unrefuted. A commit that was never recorded and a + # read that failed are both `unknown`, and clearing a block on either hands a + # merge gate away with nothing having checked the diff it now guards. + # + # This is where the approval's rule must not be reused. That one treats + # `unknown` as good enough because a wrong approval is corrected by the next + # review. A wrongly cleared block is not: nothing in this workflow puts a + # block back, so the cost lands on a human. Leaving it standing is the + # direction that heals -- the next review clears it. + if [ "$head_state" != same ]; then + case "$head_state" in + moved) echo "::warning::$REPO#$PR moved while this review ran, so it does not clear an earlier block; that block guards a commit this review did not read, and the next review clears it" ;; + *) echo "::warning::this run could not confirm which commit $REPO#$PR holds, so it does not clear an earlier block; a block cleared on an unconfirmed head needs a human to put back, and the next review that can confirm one clears it" ;; + esac + exit 0 + fi + # A driver that writes no counts leaves the three states indistinguishable, and + # the withdrawal proceeds rather than stopping. Stopping strands the block + # behind every re-review whose findings are all non-blocking, which that driver + # also concludes neutral, and which is what most re-reviews find. + if [ "$conclusion" != "success" ] && [ "$counted" = false ]; then + echo "::warning::no finding counts beside a $conclusion conclusion on $REPO#$PR, so a review that found only notes reads the same as one that read nothing; the withdrawal proceeds on the conclusion alone. A driver from v0.11.0 writes the counts" + fi + # Listed into a variable, and read from it rather than through a pipe. A + # report written into a pipe the loop reads is not an annotation Actions + # sees: it is a line the loop takes for a review id, and the dismiss fails + # on it. Reporting and data have to travel separately. + # + # A list this step cannot read is a block it cannot find, which reads the + # same on the pull request as a block it failed to clear. + # + # The marker reaches jq through the environment, the way the guard passes + # it. A value carrying a quote or a backslash is data to jq here, where + # substituting it into the program would make it syntax. + # shellcheck disable=SC2016 # $ENV is jq's own, and jq reads it + # The identity test belongs here for the reason the resolve step gives for + # its own: a dismissal is a mutation, and it stays as narrow as the identity + # that performed the thing being undone. Without it the marker alone selects, + # and a human who opens their blocking review by quoting this tool's previous + # verdict has that review dismissed by the next run -- a larger mutation than + # closing a thread, done to somebody else's merge gate. + # + # Both logins, as the history read admits both: the position posts under the + # App identity where a caller configured one and under this workflow's own + # where it did not, so a block left by either is this tool's to withdraw. + # shellcheck disable=SC2016 # $ENV is jq's own, and jq reads it + if ! ids="$(gh api "repos/$REPO/pulls/$PR/reviews" --paginate \ + --jq '[.[] | select(.state == "CHANGES_REQUESTED" + and ((.body // "") | startswith($ENV.VERDICT_MARKER)) + and ((.user.login // "") + | . == $ENV.REVIEWER_LOGIN or . == $ENV.WORKFLOW_LOGIN))] | .[].id')"; then + echo "::error::could not list the reviews to withdraw on $REPO#$PR; an earlier block may still stand and only a human can clear it" + exit 1 + fi + # Every id is tried before the step reports. Exiting inside the loop would + # leave a block standing that the next call would have cleared, and would + # name one stuck gate where there are two. + withdrawn=0 + stuck=0 + while read -r id; do + [ -n "$id" ] || continue + if gh api -X PUT "repos/$REPO/pulls/$PR/reviews/$id/dismissals" \ + -f message="Superseded: the latest review found nothing blocking in this change." \ + -f event=DISMISS >/dev/null; then + withdrawn=$((withdrawn+1)) + echo "withdrew review $id" + else + stuck=$((stuck+1)) + echo "::error::could not withdraw review $id on $REPO#$PR; it still blocks the merge on a finding this run did not reproduce. On a protected branch the API takes a dismissal only from a repository admin or an actor named in the branch's dismissal restriction, so check that this run's identity is one of them. A token without pull-requests: write is refused as well. Dismiss review $id by hand to clear it" + fi + done <<< "$ids" + echo "superseded blocks: $withdrawn withdrawn, $stuck still standing" + if [ "$stuck" -gt 0 ]; then + exit 1 + fi + + - name: Answer the request + # id: the withdrawal step at the end of this job reads this step's outcome to + # tell a run that already touched the trigger comment from one that never did. + id: answer + # The verdict, on the comment that asked for it, so the person who asked reads + # the outcome where they asked. The eyes at the top of this job say it started; + # this step withdraws them and says how it ended. + # + # Its own step, and not part of the one that records the position: that step + # is skipped for a run that reached no verdict, and the person who asked is + # owed an answer even then. Everything here needs only GITHUB_TOKEN and the + # job's issues: write. + # + # The condition names three facts: a review turn, not cancelled, and a comment + # to answer. A close produces no verdict, so a request to tear a session down + # earns no answer. + # + # !cancelled() is what keeps a thumb honest, and it is not interchangeable with + # always() here. A cancellation arriving after `drive` finishes leaves + # check_path and verdict_produced populated, so this step would read a real + # conclusion and thumb the request -- while `Post the verdict` below skips and + # the verdict never reaches the pull request. A thumb reads as an answer, so + # that is worse than no reaction at all. The last step of this job withdraws + # the eyes a cancelled run leaves, and it cannot reach a conclusion to post. + # + # No verdict_produced gate: the acknowledgement waits on this step, and a re-run + # replays the trigger comment id, so a run reaching no verdict still has to clear + # the eyes, and a thumb an earlier attempt left there. + if: ${{ inputs.mode == 'review' && !cancelled() + && needs.guard.outputs.comment_id != '' }} + continue-on-error: true + shell: bash + env: + GH_TOKEN: ${{ github.token }} + # The repository the request was written on, which is the one the review + # ran on. + TRIGGER_REPO: ${{ github.repository }} + TRIGGER_ID: ${{ needs.guard.outputs.comment_id }} + # The same collection `Acknowledge the trigger` posted the eyes to. Reading + # a different one leaves them standing. + COMMENT_API: ${{ needs.guard.outputs.comment_api }} + CHECK: ${{ steps.drive.outputs.check_path }} + # Whether there is a verdict to react to. See the conclusion below. + VERDICT_PRODUCED: ${{ steps.drive.outputs.verdict_produced }} + run: | + set -euo pipefail + # An absent check file reads as an absent conclusion, which the case below + # answers by clearing. Returning here leaves the eyes standing, and a thumb an + # earlier attempt left, for a run that reached no verdict. + # + # A run that produced no verdict is read as no conclusion, whatever the file + # holds. It holds `failure` there, and that is the same word a review carrying + # blockers concludes -- so reading it here would thumb down a run that reviewed + # nothing, which says the change is bad when nobody could read it. The title + # separates the two in the checks list; a reaction has no title, so this reads + # the gate the workflow already computed instead. + conclusion="" + if [ -s "$CHECK" ] && [ "${VERDICT_PRODUCED:-}" = "true" ]; then + # jq's error goes to the log and the value comes back on stdout, so a check + # file this step cannot read states itself rather than arriving as data. + conclusion="$(jq -r '.conclusion // empty' "$CHECK" || true)" + fi + + # On the conclusion, not on the position the step above records: a repository + # that has not opted into approve-on-success records none for a clean review, + # and that reader still asked a question. + # + # neutral earns nothing, and neither does a conclusion this step cannot read. + # neutral is what a review concludes when it cannot show it read the diff, and + # a thumb up there says the change is fine when nobody looked at it. + # + # The reaction and the ones it replaces are chosen in one statement, so the + # two cannot drift apart. + # + # Every arm withdraws the eyes. The acknowledgement is a promise of an answer, + # and this step is where the answer arrives, so the arm that reaches no verdict + # withdraws them too -- and with them both thumbs. The comment carries what the + # last run left there, so a run that reaches neither verdict would otherwise + # leave the request wearing a verdict this run did not reach. + case "$conclusion" in + failure) reaction="-1"; stale="+1 eyes" ;; + success) reaction="+1"; stale="-1 eyes" ;; + *) + echo "a ${conclusion:-missing} conclusion earns no reaction; the verdict comment carries what it found" + reaction=""; stale="+1 -1 eyes" ;; + esac + + # Withdraw this bot's stale reactions first: the eyes that acknowledged the + # trigger, and a thumb an earlier attempt left. A reaction is not a toggle and + # a re-run replays the same comment id, so a comment would otherwise wear a + # verdict this run did not reach. Scoped to the reacting identity: a human who + # thumbed the request down is voicing an opinion, and it is not this job's to + # delete. The same scope leaves a human's eyes alone. + # + # GITHUB_TOKEN reacts as github-actions[bot], and an installation token cannot + # ask the API which login it carries, so the login is named here. + me="github-actions[bot]" + # Listed into a variable, and read from it rather than through a pipe, so + # nothing this block reports can be read back as a reaction id. Paginated, + # because a busy comment carries more reactions than one page holds and a + # miss leaves the request wearing the eyes, or a thumb this run replaces. + if ! mine="$(gh api "repos/$TRIGGER_REPO/$COMMENT_API/$TRIGGER_ID/reactions" \ + --paginate \ + --jq ".[] | select(.user.login == \"$me\") | \"\\(.content) \\(.id)\"")"; then + echo "::warning::could not read the reactions on comment $TRIGGER_ID in $TRIGGER_REPO; the eyes or a stale thumb from this bot may stay on it" + mine="" + fi + while read -r content rid; do + [ -n "$rid" ] || continue + case " $stale " in *" $content "*) ;; *) continue ;; esac + if gh api -X DELETE \ + "repos/$TRIGGER_REPO/$COMMENT_API/$TRIGGER_ID/reactions/$rid" \ + >/dev/null; then + echo "withdrew this bot's $content from comment $TRIGGER_ID" + else + echo "::warning::could not withdraw this bot's $content from comment $TRIGGER_ID in $TRIGGER_REPO" + fi + done <<< "$mine" + + # Nothing to post when the conclusion earned no reaction: the clearing above is + # the whole answer there. + [ -n "$reaction" ] || exit 0 + + # Never fatal: a reaction is a courtesy, and losing one must not fail a review + # that ran and published. Idempotent per identity and content, so a re-run on + # the same comment returns the reaction already there rather than a second one. + if gh api -X POST "repos/$TRIGGER_REPO/$COMMENT_API/$TRIGGER_ID/reactions" \ + -f content="$reaction" >/dev/null; then + echo "reacted $reaction on comment $TRIGGER_ID in $TRIGGER_REPO" + else + echo "::warning::could not react on comment $TRIGGER_ID in $TRIGGER_REPO; the review stands" + fi + + - name: Post the verdict + # id: the withdrawal step at the end of this job reads this step's `posted` + # output to decide whether the trigger comment already carries an honest answer. + id: verdict + # Post only when a real verdict was produced, and even when the drive + # step above exited non-zero -- `!cancelled()` runs on any outcome + # except the job itself being cancelled (e.g. superseded by a newer + # `seidroid review`), which is the one case with nothing to post. + # Keying on verdict_produced, not the exit code, so a teardown-only + # failure still posts a valid verdict and a no-verdict run never + # upserts a placeholder. + # + # continue-on-error. This step is the last thing standing between a finished + # review and the reader, and failing the job here throws that review away + # rather than saving it. What it costs is the signal, so the run block states + # the failure itself; see there. + if: ${{ inputs.mode == 'review' && (!cancelled() && steps.drive.outputs.verdict_produced == 'true') }} + continue-on-error: true + shell: bash + env: + GH_TOKEN: ${{ steps.identity.outputs.token || github.token }} + # The reviewed pull request, which is where a reader looks for a review. + REPO: ${{ github.repository }} + PR: ${{ needs.guard.outputs.pr_number }} + VERDICT: ${{ steps.drive.outputs.verdict_path }} + # Findings the step above could place nowhere on the diff. Empty when it + # placed them all. + NOTE: ${{ steps.place.outputs.note_path }} + # What the driver counted, for the findings line below. Read rather than + # recomputed here: the driver derived the check run's conclusion from these + # same findings, and a second derivation is a second thing that can disagree. + CHECK: ${{ steps.drive.outputs.check_path }} + # What the step above placed. Empty, not zero, when that step died partway -- + # and the difference is the point. "0 posted inline" over a placement that + # never ran is a lie about the review. + ON_LINE: ${{ steps.place.outputs.on_line }} + ON_FILE: ${{ steps.place.outputs.on_file }} + UNPLACED: ${{ steps.place.outputs.unplaced }} + # Whether the review carrying the inline comments took the verdict, and which + # review that is. Together they decide what this step does: append to that + # review, or post the verdict on its own because no review holds it. + SUMMARY_POSTED: ${{ steps.place.outputs.summary_posted }} + REVIEW_ID: ${{ steps.place.outputs.review_id }} + # The commit the review read, for the failure check below. Recorded before + # the review started; absent only when that read failed, and then the + # annotation is the only record. + REVIEWED_SHA: ${{ steps.head.outputs.sha }} + # Read for the failure check below, which publishes under the same names the + # publish step does. One switch drives both, so the set of names matches. + AI_REVIEW_COPY: ${{ inputs.publish-ai-review-check }} + # GitHub rejects an issue comment over 65,536 characters. The driver bounds + # the verdict it writes and clips its own text to fit (review.MaxBodyBytes, + # 60,000). Nothing bounds $NOTE: it carries Finding.Detail, raw model prose + # with no length constraint on it. Unbounded, a long tail of unplaced + # findings pushes the body past the cap and the upsert below is rejected -- + # losing a whole review over its least important part. + # + # Bytes, not characters, for the reason the driver counts bytes: a byte count + # is never lower than a rune count, so a body inside this bound is inside + # GitHub's limit whichever unit that limit turns out to count. + MAX_BODY_BYTES: 65536 + # Held back from the note's budget for the truncation notice, which is + # written after the cut point is chosen and so cannot be measured before it. + NOTICE_BYTES: 256 + run: | + set -euo pipefail + body="$VERDICT_MARKER"$'\n'"$(cat "$VERDICT")" + + # The findings line, in the shape ai-review posts, so a reader moving between + # the two reviewers during the transition reads one format. Assembled here + # rather than in the driver because half of it is the placement above, which + # the driver never sees. + # + # It is appended before the note block below, and that ordering is load-bearing + # twice. It puts the line under the review's prose, where the format wants it. + # And the note's byte budget measures $body to decide what room is left, so a + # line added after that measurement is a line nothing accounted for -- and what + # it pushes past GitHub's cap is the whole review. + # + # Every term is dropped rather than guessed, and nothing here can fail the + # publish. A count this run cannot read costs the term; a line with no count + # left in it is not written at all. + is_count() { case "${1:-}" in (''|*[!0-9]*) return 1 ;; esac; } + line="" counted=false + add_term() { if [ -n "$line" ]; then line="$line | "; fi; line="$line$1"; } + + # The driver's own totals, over every finding the review reported rather than + # over the ones that could be placed: a blocker naming no line is still a + # blocker, and a count that omitted it would read as a cleaner review than the + # one that ran. + # + # A driver older than these fields writes none of them and jq answers null. The + # sentinel makes that a value is_count rejects, so an old driver publishes the + # same comment with a shorter line that is still true. + # Under `counts`, which is where the driver writes them: check.json carries + # `counts: {blocking, non_blocking, placeable, pre_existing}` beside a + # top-level `conclusion`. Read at the ROOT they answered null on every run, so + # the line never printed a number, the notice below claimed the driver predates + # fields it has, and neither disagreement warning could fire -- the one automated + # cross-check between this comment and the check run was dead code. + blocking="" non_blocking="" pre_existing="" conclusion="" + if [ -s "${CHECK:-}" ]; then + read -r blocking non_blocking pre_existing conclusion < <(jq -r '[(.counts.blocking // "?"), + (.counts.non_blocking // "?"), (.counts.pre_existing // "?"), (.conclusion // "?")] | @tsv' \ + "$CHECK" 2>/dev/null) || true + fi + if is_count "$blocking" && is_count "$non_blocking"; then + add_term "$blocking blocking" + add_term "$non_blocking non-blocking" + counted=true + else + echo "::notice::this driver reports no finding counts, so the findings line \ + omits them; it predates check.json's blocking and non_blocking fields" + fi + + # Both placements the step above makes: on a line where the diff carries it, on + # the file where it does not. Both land in the pull request's file view, which + # is what "inline" means to the person reading it. What reached neither is not + # counted here -- the note below names those one by one under a heading of their + # own, which is more than a number would say. + if is_count "${ON_LINE:-}" && is_count "${ON_FILE:-}" && is_count "${UNPLACED:-}"; then + add_term "$(( ON_LINE + ON_FILE )) posted inline" + counted=true + else + add_term "inline placement did not run" + echo "::notice::no placement counts, so the findings line says so; that step \ + writes none of them if it dies partway" + fi + + # Counted apart from both, and named. CheckConclusion excludes a pre-existing + # blocker from the gate on purpose -- it is already on the base branch, so + # failing on it would fail every pull request that touches the file, and the + # author who has to clear the check is the one person who did not cause it. + # Folded into "blocking" it would print a number the check run contradicts; + # folded into "non-blocking" it would tell an author their change has problems + # it does not have. Omitted at zero, which is most reviews. + if is_count "$pre_existing" && [ "$pre_existing" -gt 0 ]; then + add_term "$pre_existing pre-existing" + fi + + # Two numbers about one review must not disagree. Both are the driver's, from + # the same findings, so a disagreement is a defect there rather than a case to + # render around. It is stated in the log and the line still prints what it was + # given: correcting either number here would publish a third answer and hide + # the bug that produced the first two. + if is_count "$blocking" && [ "$conclusion" = "failure" ] && [ "$blocking" -eq 0 ]; then + echo "::warning::the driver reported 0 blocking findings beside a failing \ + check; this comment and the check run disagree" + elif is_count "$blocking" && [ "$conclusion" != "failure" ] && [ "$blocking" -gt 0 ]; then + echo "::warning::the driver reported $blocking blocking finding(s) beside a \ + $conclusion check; this comment and the check run disagree" + fi + + if [ "$counted" = true ]; then + body="$body"$'\n\n'"**Findings:** $line" + fi + + # A reader who sees no inline comment for an observation would otherwise + # have to guess whether it was dropped. + # + # Bounded against what the marker and the verdict already spend, so the note + # can never be the thing that pushes the comment past GitHub's cap. Cut on + # whole lines: a byte cut can land inside a UTF-8 sequence or inside a + # finding's markdown, where a line cut can be counted and named. + if [ -n "$NOTE" ] && [ -s "$NOTE" ]; then + room=$(( MAX_BODY_BYTES - $(printf '%s\n\n' "$body" | wc -c) )) + if [ "$(wc -c < "$NOTE")" -le "$room" ]; then + body="$body"$'\n\n'"$(cat "$NOTE")" + else + # Every line the place step writes for a finding opens with "- `". + total="$(grep -c '^- `' "$NOTE" || true)" + budget=$(( room - NOTICE_BYTES )) + kept="" + shown=0 + if [ "$budget" -gt 0 ]; then + # sed drops the last line, which is the one the byte cut may have left + # half-written. + kept="$(head -c "$budget" "$NOTE" | sed '$d')" + shown="$(printf '%s\n' "$kept" | grep -c '^- `' || true)" + fi + if [ "$shown" -gt 0 ]; then + body="$body"$'\n\n'"$kept" + fi + body="$body"$'\n\n'"_Cut to stay inside GitHub's comment limit ($MAX_BODY_BYTES bytes): $shown of $total observation(s) are shown here. All $total are in this workflow run's log._" + echo "::warning::the summary note was cut to $shown of $total unplaced observation(s) to fit GitHub's comment limit; the full list follows" + cat "$NOTE" + fi + fi + # A NEW comment every iteration, never an edit of the last one. Editing leaves + # the comment at its ORIGINAL position in the thread and notifies nobody, so a + # re-review became close to invisible: it did not move, it did not notify, and + # it destroyed the previous verdict's text. A verdict written four minutes ago + # rendered eighteen hours up the page, indistinguishable from a review that + # never ran, and that cost real diagnosis time. + # + # Earlier verdicts are left standing on purpose. They are the record of what the + # review said before the author's fixes, which is the thing a reader compares + # against. A long pull request accumulates a few, which is the accepted cost. + # + # It also removes a hazard the marker lookup carried: this marker is shared with + # the unplaced-findings note posted above, so selecting `.[0]` by marker could + # overwrite that note rather than the previous verdict. + # + # repo/pr from env, not template-interpolated into the script. The failure is + # caught rather than left to `set -e`, so this step can say what was lost. + # None of that applies to the append below. It edits a review this run created + # seconds ago, which is already the newest thing on the pull request and has + # already notified: the reader has not read it yet, so there is no earlier text + # to destroy and no position to lose. What the edit adds is what the placement + # step could not know when it posted -- how many findings reached the code, and + # which ones reached none of it. + # + # The comment is the fallback and not the default. A verdict in the review AND + # a verdict in a comment is the duplication PLT-1268 asked us to remove, so + # exactly one of these two paths runs. + posted=false + if [ "${SUMMARY_POSTED:-}" = "true" ]; then + # The verdict is on the pull request, whatever the append does next. + posted=true + if [ -n "${REVIEW_ID:-}" ]; then + if gh api -X PUT "repos/$REPO/pulls/$PR/reviews/$REVIEW_ID" \ + -f body="$body" >/dev/null 2>&1; then + echo "the review carrying the findings on $REPO#$PR now carries the whole verdict" + else + echo "::warning::could not append the findings line and the unplaced observations to review $REVIEW_ID on $REPO#$PR; the verdict that review carries still stands, and what did not reach it follows" + echo "--- what the review does not carry ---" + printf '%s\n' "$body" + echo "--- end ---" + fi + else + echo "::warning::the review carrying the verdict on $REPO#$PR could not be named, so the findings line and the unplaced observations did not reach it; what did not reach it follows" + echo "--- what the review does not carry ---" + printf '%s\n' "$body" + echo "--- end ---" + fi + elif gh api -X POST "repos/$REPO/issues/$PR/comments" -f body="$body" >/dev/null; then + posted=true + echo "no review carried the verdict on $REPO#$PR, so it is posted as a comment of its own" + fi + # Whether the verdict is ON the pull request, for the withdrawal step at the + # end of this job: a thumb on the trigger comment answers a verdict only when + # the verdict is there to read. This step's exit status cannot carry that. The + # failure path below ends on a call whose failure it swallows, so the step + # exits 0 whether the comment landed or not, and its outcome reads success + # either way. + echo "posted=$posted" >> "$GITHUB_OUTPUT" + if [ "$posted" = true ]; then + # Earlier runs may have left notices saying this review did not complete. + # It did, so that notice now asserts a state that is not true and links a run + # that is not the latest. The check run supersedes itself on the new commit; + # a comment does not, so this withdraws it. + # + # Matched on the marker at the start of the body, and only on this tool's own + # comments, so a person quoting the marker cannot have their comment deleted. + # The listing emits one id per line: --paginate runs the filter once per page, + # so aggregating inside jq would emit one result per page and concatenate them + # into a string no comment carries. + # + # Every failure here is tolerated. The verdict is posted, and a stale notice + # beside it is a smaller cost than failing a step that already did its work. + # shellcheck disable=SC2016 # $ENV is jq's, and single quotes are what keep it jq's + # Every one, not the newest. Deleting one per run only holds the + # "exactly one notice" invariant while no deletion has ever failed, and + # that path is tolerated here -- so a single refusal leaves a notice that + # no later run reaches, standing beside a verdict that did land. + stale_notices="$(gh api --paginate "repos/$REPO/issues/$PR/comments?per_page=100" \ + --jq '.[] | select(.user.type == "Bot") | select(.body | startswith($ENV.NO_VERDICT_MARKER)) | .id' \ + 2>/dev/null || true)" + while read -r stale_notice; do + [ -n "$stale_notice" ] || continue + if gh api -X DELETE "repos/$REPO/issues/comments/$stale_notice" >/dev/null 2>&1; then + echo "withdrew the no-verdict notice $stale_notice an earlier run left on $REPO#$PR" + else + echo "::warning::could not withdraw the no-verdict notice $stale_notice on $REPO#$PR; it now says this review did not complete, and it did" + fi + done <<< "$stale_notices" + exit 0 + fi + + # continue-on-error above is what stops a publishing failure from discarding a + # review that ran. It also removes the only signal there was, and a review that + # is silently absent is worse than a red job. So the failure is stated in two + # places, because neither alone is enough: an annotation on the run, which needs + # nothing but the runner; and the check run, which is the only one of the two + # that reaches the pull request, where the reader is waiting for a review that + # is not coming. + # + # The verdict goes to the log unposted, so the run still holds what the review + # cost model spend and a sandbox to produce. + bytes="$(printf '%s' "$body" | wc -c | tr -d '[:space:]')" + echo "::error::the review reached a verdict but it could not be posted on $REPO#$PR ($bytes bytes); it is in this step's log below" + echo "--- verdict, unposted ---" + printf '%s\n' "$body" + echo "--- end verdict ---" + # Under every name the publish step above writes. This conclusion then + # supersedes each of them on this commit rather than sitting beside one, and a + # later run that does post clears them. A copy left carrying the earlier + # conclusion is a rule reading green on a verdict that never arrived. + # + # `review` goes first and is the check of record. The `AI Review` copy follows + # it, under the switch that step reads, so the two publish the same set of + # names on both paths. + # + # Best-effort, both of them: whatever stopped the comment can stop this too, + # and then the annotation stands alone. + if [ -n "${REVIEWED_SHA:-}" ]; then + fail_check() { + gh api -X POST "repos/$REPO/check-runs" \ + -f name="$1" \ + -f head_sha="$REVIEWED_SHA" \ + -f status=completed \ + -f conclusion=failure \ + -f output[title]="review produced but not published" \ + -f output[summary]="The review ran and reached a verdict. Posting it to this pull request failed, so the verdict is not here. Read it in the workflow run: $GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" \ + >/dev/null + } + fail_check review \ + || echo "::warning::the failure check run could not be posted either; the annotation on this run is the only record" + if [ "${AI_REVIEW_COPY:-}" = "true" ]; then + fail_check "AI Review" \ + || echo "::warning::the AI Review copy of the failure check run could not be posted on $REPO#$PR; it still carries this review's earlier conclusion" + fi + else + echo "::warning::no reviewed commit was recorded, so there is no check run to fail; the annotation on this run is the only record" + fi + + - name: Resolve the threads this review closed + # A finding that still holds gets a new thread on every re-review while the old + # one stays open. On one live pull request 5 of 13 threads were byte-identical + # repeats of two findings, so a third re-review has the author read one finding + # in three places and dismiss it three times. The driver names which of its own + # threads this review closed; this closes them. + # + # Two lists, two gates. `addressed` is a finding the diff no longer shows, so it + # closes whenever the review published. `superseded` is a finding restated as a + # new comment, so it closes only once the comment restating IT reached the code + # -- a thread shut behind a comment that never posted takes a live finding off + # the pull request and puts nothing where it was. + # + # That second gate is per THREAD. Each finding names the threads its comment + # replaces, placement records those ids as the comment posts, and a thread here + # closes on finding its id in that record. So a review superseding three threads + # closes the one whose replacement reached the code and leaves the other two + # standing beside the findings that restate them. + # + # A driver that publishes no linkage falls back to the per-review gate: every + # superseded thread together, and only when placement dropped nothing at all. + # Coarse in the safe direction -- one unplaced comment holds all of them open -- + # and placement's superseded_linked output is what selects between the two. + # + # The record is what a comment DID, and `addressed` is what publication alone + # earns. So an `addressed` id needs no record and takes neither gate. + # + # Last of the publishers, so nothing old closes before the new review is on the + # pull request. + # + # Every id is checked here as well as in the driver. The driver refuses an id + # that matches no thread it was handed; this refuses one that is not, right now, + # an unresolved thread this reviewer wrote. The mutation is on someone else's + # pull request and the ids come from model output, so the check runs beside the + # call rather than only upstream of it. + # + # REVIEWER_LOGIN alone, where the history read admits WORKFLOW_LOGIN beside it. + # Reading a thread this run did not write costs nothing; closing one is a + # mutation, and it stays as narrow as the identity performing it. + # + # continue-on-error, like every publisher in this job. A review that ran and + # published must not be failed over a thread it could not close: the cost of not + # closing one is the duplicate this workflow leaves today. + # + # resolveReviewThread is a pull-request operation. GitHub categorises it under + # pulls and publishes no permission requirement of its own for it, and this job + # grants pull-requests: write. So a refusal names the identity rather than the + # thread, and the warning below reports which identity the API refused. + # + # Every page, as the history read takes them. The two have to see the same pull + # request: a resolver stopping at the first page would call a thread the history + # carries one this tool never left. + if: ${{ inputs.mode == 'review' && !cancelled() + && steps.drive.outputs.verdict_produced == 'true' }} + continue-on-error: true + shell: bash + env: + GH_TOKEN: ${{ steps.identity.outputs.token || github.token }} + REPO: ${{ github.repository }} + PR: ${{ needs.guard.outputs.pr_number }} + # The driver's plan, in the file this job already reads to publish the check + # run. An older driver writes no `threads` key, and this step then closes + # nothing -- which is what the workflow did before it could. + CHECK: ${{ steps.drive.outputs.check_path }} + # Which threads had their replacement reach the code, one id per line, and + # whether the driver published that linkage at all. Absent when placement did + # not run, and the fallback below refuses on its own terms there. + LINKAGE: ${{ steps.place.outputs.linkage_path }} + PLACED_LINKED: ${{ steps.place.outputs.superseded_linked }} + # What placement managed, and what it could not place. Read by the fallback + # gate alone: they are counts over the whole review and say nothing about + # which comment replaced which thread. + PLACED_ON_LINE: ${{ steps.place.outputs.on_line }} + PLACED_ON_FILE: ${{ steps.place.outputs.on_file }} + PLACED_UNPLACED: ${{ steps.place.outputs.unplaced }} + run: | + set -euo pipefail + # What the API said, on one line and bounded. A warning that reports only + # "could not be resolved" tells an operator nothing to act on, and the message + # is the difference between a thread somebody else already touched and a + # credential that cannot do this at all. + saidIt() { printf '%s' "${1:-}" | tr '\n\r' ' ' | cut -c1-300; } + apiSaid() { saidIt "$(cat "$1" 2>/dev/null)"; } + + if [ ! -s "${CHECK:-}" ]; then + echo "no check file, so nothing names a thread to close" + exit 0 + fi + + # The driver already refused these against the history it was handed. Repeated + # here because this is where a person reading the run sees it, and an invented + # id means the review is naming threads that do not exist. + while IFS= read -r id; do + [ -z "$id" ] && continue + echo "::warning::the review named review thread '$id', which is not one this tool left on $REPO#$PR; it was not resolved" + done < <(jq -r '.threads.refused // [] | .[]' "$CHECK") + + wanted="$(jq -r '.threads.addressed // [] | .[]' "$CHECK")" + held="$(jq -r '.threads.superseded // [] | length' "$CHECK")" + + # The plan is the warrant and the record is which comment spent it, so a + # thread has to be in both. The plan alone would close one the record cannot + # account for; the record alone would close one the driver refused. + # + # An id nowhere in the record is a replacement that did not reach the code, + # whatever happened to the rest of the review. That is the whole of the gate: + # no count over the review enters it, because no count can say which comment + # replaced which thread. + if [ "${PLACED_LINKED:-}" = "true" ]; then + landed="${LINKAGE:-}" + [ -s "$landed" ] || landed=/dev/null + closing=0 + while IFS= read -r id; do + [ -z "$id" ] && continue + if grep -qxF -- "$id" "$landed"; then + wanted="$wanted"$'\n'"$id" + closing=$((closing+1)) + continue + fi + echo "review thread $id stays open: nothing replacing it reached the code" + done < <(jq -r '.threads.superseded // [] | .[]' "$CHECK") + if [ "$held" -gt 0 ]; then + echo "superseded: $closing of $held thread(s) had their own replacement reach the code" + fi + else + # The fallback, for a driver that publishes no linkage. Both halves are + # required. Counting only what landed lets one unrelated new finding on a + # line stand in for three superseded replacements that landed nowhere -- and + # those three threads would close with the findings that replaced them + # sitting in the summary instead of on the diff. + # + # The default in ${PLACED_UNPLACED:-1} is deliberate and is not a count. An + # absent output means placement did not report, which has to read as + # "something may be unplaced" rather than as zero: this gate decides whether + # a live finding comes off the pull request, so the unknown falls on the side + # that leaves the thread open. + placed=$(( ${PLACED_ON_LINE:-0} + ${PLACED_ON_FILE:-0} )) + if [ "$placed" -gt 0 ] && [ "${PLACED_UNPLACED:-1}" -eq 0 ]; then + wanted="$wanted"$'\n'"$(jq -r '.threads.superseded // [] | .[]' "$CHECK")" + elif [ "$held" -gt 0 ]; then + echo "$held superseded thread(s) stay open: $placed comment(s) reached the code and ${PLACED_UNPLACED:-an unreported number} could not be placed" + fi + fi + wanted="$(printf '%s\n' "$wanted" | sed '/^$/d' | sort -u)" + if [ -z "$wanted" ]; then + echo "this review closes no thread" + exit 0 + fi + + owner="${REPO%%/*}"; name="${REPO##*/}" + state="$RUNNER_TEMP/review-resolve-threads.json" + # Read again here rather than reused from the history step. Minutes of review + # sit between the two, and a thread an author resolved in that time is one this + # step must not report as its own doing. + # + # Every page, like the history read. The two have to see the same pull request: + # the history carries a thread from page two, the review can name it, and a + # resolver that stopped at the first page would report a thread this tool did + # leave as one it never wrote -- a false statement in the log, and the thread + # left open beside the finding that restates it. + # + # $owner and friends are GraphQL variables, so the query stays literal. + # shellcheck disable=SC2016 + if ! gh api graphql --paginate -f query=' + query($owner: String!, $name: String!, $number: Int!, $endCursor: String) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + reviewThreads(first: 100, after: $endCursor) { + nodes { + id + isResolved + comments(first: 1) { nodes { body author { login } } } + } + pageInfo { hasNextPage endCursor } + } + } + } + }' -F owner="$owner" -F name="$name" -F number="$PR" 2> "$state.err" \ + | jq -s '[ .[].data.repository.pullRequest.reviewThreads.nodes[] ]' > "$state"; then + echo "::warning::the review threads on $REPO#$PR could not be read as ${REVIEWER_LOGIN:-an unnamed identity}: $(apiSaid "$state.err"); no thread was closed and the author sees each restated finding twice" + exit 0 + fi + + # Three sets, because a thread this run will not close is not one thing. Only + # the last of them is a warning, and saying so takes all three: a message that + # calls a thread this tool wrote one it never left is false, whichever reason + # kept the run from closing it. + # + # open -- this run's own identity, still unresolved. The only set it closes. + # ours -- this run's own identity, any state. Already resolved, nothing to do. + # either -- this tool's, under either identity it posts as. The history read + # admits these, so the review can legitimately name one; this run + # does not close a thread it did not write, and says which identity + # did instead of denying the thread. + # + # The mutation keeps the strict single-login test. Reading a thread another + # identity wrote is inert; closing one is not, and it stays as narrow as the + # identity performing it. + # shellcheck disable=SC2016 # $ENV is jq's, and single quotes are what keep it jq's + marked='.[] | select((.comments.nodes[0].body // "") | startswith($ENV.FINDING_MARKER))' + # shellcheck disable=SC2016 # same + mine=' | select((.comments.nodes[0].author.login // "") == $ENV.REVIEWER_LOGIN)' + # shellcheck disable=SC2016 # same + anyOfOurs=' | select(.comments.nodes[0].author.login + | . != null and . != "" + and (. == $ENV.REVIEWER_LOGIN or . == $ENV.WORKFLOW_LOGIN))' + ours="$(jq -r "$marked$mine | .id" "$state")" + open="$(jq -r "$marked$mine | select(.isResolved | not) | .id" "$state")" + either="$(jq -r "$marked$anyOfOurs | .id" "$state")" + + closed=0 refused=0 failed=0 elsewhere=0 + while IFS= read -r id; do + [ -z "$id" ] && continue + if ! printf '%s\n' "$open" | grep -qxF -- "$id"; then + if printf '%s\n' "$ours" | grep -qxF -- "$id"; then + echo "review thread $id is already resolved; nothing to do" + elif printf '%s\n' "$either" | grep -qxF -- "$id"; then + # This tool left it, under the identity it was posting as then. The + # history read admits both, so naming it is correct; closing it is not + # this run's to do, and calling it a thread this tool never wrote would + # be false. + echo "review thread $id on $REPO#$PR was left under this tool's other identity, not ${REVIEWER_LOGIN:-the one this run uses}; this run does not close it and it stays open" + elsewhere=$((elsewhere+1)) + else + echo "::warning::review thread '$id' is not an unresolved thread this tool left on $REPO#$PR; it was not resolved" + refused=$((refused+1)) + fi + continue + fi + # Both streams are captured, because gh splits a failure across them: the + # API's error body goes to stdout and gh's own line to stderr, and either + # alone can be the half that says why. Discarded on success. + # shellcheck disable=SC2016 # $threadId is GraphQL's + if out="$(gh api graphql -f query=' + mutation($threadId: ID!) { + resolveReviewThread(input: {threadId: $threadId}) { thread { id isResolved } } + }' -F threadId="$id" 2>&1)"; then + closed=$((closed+1)) + else + echo "::warning::review thread $id on $REPO#$PR was not resolved as ${REVIEWER_LOGIN:-an unnamed identity}: $(saidIt "$out"); it stays open and the author reads this finding twice" + # A refusal is about the identity rather than the thread, and which + # identity ran decides what to change. An App installation token carries + # the App's own permissions and this job's permissions block does not + # narrow it; the workflow token carries only what that block granted. + case "$out" in + *"not accessible by integration"*|*Forbidden*|*"HTTP 403"*|*"must have"*) + echo "::warning::that refusal names the identity, not the thread. ${REVIEWER_LOGIN:-the identity this run used} was refused resolveReviewThread on $REPO#$PR: an App identity carries the App installation's permissions, and github-actions carries only what this job's permissions block grants" ;; + esac + failed=$((failed+1)) + fi + done <<< "$wanted" + echo "threads: $closed closed, $refused refused, $elsewhere left under another identity, $failed could not be resolved" + + - name: Report a review that reached no verdict + # The step above answers a review that decided; this one answers a review that + # could not be read. A person who asked would otherwise get a red job, an + # untouched pull request, and one line in a log nobody opens. ai-review.yml + # answers the same case the same way, and this is parity with it. + # + # The complement of the step above, on one output: `== 'true'` there, `!= 'true'` + # here. So a review run that was not cancelled posts one comment or the other, + # never both and never neither -- including a run that died before the driver, + # where verdict_produced is unset and this is the only record left. + # + # It reports and takes no position. No approval, no request for changes, and no + # withdrawal of an earlier block: the step above owns all three, and it keeps + # the verdict gate for that reason. + # + # No byte-cap arithmetic, unlike the step above. What it appends is a bounded + # summary the driver clipped, not the unbounded findings note. + # + # continue-on-error, like its sibling. The drive step already failed the job on + # this path -- it forces a non-zero exit when a review produced no verdict -- so + # failing it again here buys no signal; the annotation in the run block is what + # replaces the one it costs. + if: ${{ inputs.mode == 'review' && !cancelled() + && steps.drive.outputs.verdict_produced != 'true' }} + continue-on-error: true + shell: bash + env: + GH_TOKEN: ${{ steps.identity.outputs.token || github.token }} + # Where the verdict comment would have gone. + REPO: ${{ github.repository }} + PR: ${{ needs.guard.outputs.pr_number }} + # Why there is no verdict, in the driver's own words. Absent when the run + # never reached the driver, and the body says which of the two happened. + CHECK: ${{ steps.drive.outputs.check_path }} + run: | + set -euo pipefail + # Removes every no-verdict notice standing on the pull request, so this run's + # own is the only one left. + # + # Matched on the marker at the start of the body, and only on this tool's own + # comments, so a person quoting the marker cannot have their comment deleted. + # The listing emits one id per line: --paginate runs the filter once per page, + # so aggregating inside jq would emit one result per page and concatenate them + # into a string no comment carries. + # + # Every failure here is tolerated. Whichever caller runs this has something + # else to say afterwards, and a stale notice is a smaller cost than losing it. + delete_prior_notice() { + local ids id + # Every one, not the newest, for the reason the verdict step's copy gives: + # the "exactly one" invariant above holds only while no deletion has failed, + # and every failure here is tolerated. + # shellcheck disable=SC2016 # $ENV is jq's, and single quotes are what keep it jq's + ids="$(gh api --paginate "repos/$REPO/issues/$PR/comments?per_page=100" \ + --jq '.[] | select(.user.type == "Bot") | select(.body | startswith($ENV.NO_VERDICT_MARKER)) | .id' \ + 2>/dev/null || true)" + while read -r id; do + [ -n "$id" ] || continue + if gh api -X DELETE "repos/$REPO/issues/comments/$id" >/dev/null 2>&1; then + echo "withdrew the standing no-verdict notice $id on $REPO#$PR" + else + echo "::warning::could not withdraw the no-verdict notice $id on $REPO#$PR" + fi + done <<< "$ids" + } + + # The check run's own summary, not a second account of the same run. That + # check is published above and a reader compares the two; one string is what + # keeps them from disagreeing. jq's error goes to the log and the value comes + # back empty, so a check file this step cannot read falls to the case below. + reason="" + if [ -s "${CHECK:-}" ]; then + reason="$(jq -r '.summary // empty' "$CHECK" 2>/dev/null || true)" + fi + # Two different runs reach this. One reviewed and could not be read, and the + # driver said why. The other never got as far as the driver -- a failed + # install, a toolchain that would not resolve -- and has no reason to give, so + # this says that rather than implying the review ran. + started=true + if [ -z "$reason" ]; then + reason="This run recorded no reason, which happens when it stopped before the review started." + started=false + fi + body="$(printf '%s\n%s\n\n%s\n\n%s\n' \ + "$NO_VERDICT_MARKER" \ + '**This review did not complete.** It reached no verdict, so there is no review of this change to act on.' \ + "$reason" \ + "Read this run for the rest: $GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID")" + # One notice per pull request: every earlier one is deleted and a new one is + # posted. The verdict comment accumulates on purpose -- an author compares + # this review against the last one -- but a notice that the review did not + # complete carries no such history, and an outage on a caller wired to every + # push would leave one identical copy per push. + # + # Deleted and reposted rather than edited, for the reason `Post the verdict` + # states about itself: an edit stays where it was in the thread and notifies + # nobody, so a second consecutive failure would reach the person who just + # asked as nothing at all. Posting lands it at the bottom, carrying this run's + # url. + # + # A deletion that fails costs a duplicate notice; the post below runs either + # way, so the person who asked always gets an answer. + delete_prior_notice + if gh api -X POST "repos/$REPO/issues/$PR/comments" -f body="$body" >/dev/null; then + echo "reported a no-verdict review on $REPO#$PR" + exit 0 + fi + # The last record standing. continue-on-error above keeps a failed comment + # from adding a second failure to a job the drive step already failed, and it + # also removes the only signal there was -- so the report goes to the log + # where the run still holds it. + if [ "$started" = true ]; then + echo "::error::the review reached no verdict, and that could not be reported on $REPO#$PR; this run's log is the only record" + else + echo "::error::this run stopped before the review started, and that could not be reported on $REPO#$PR; this run's log is the only record" + fi + echo "--- report, unposted ---" + printf '%s\n' "$body" + echo "--- end report ---" + + - name: Withdraw the reactions on a cancelled run + # The one step in this job that runs on a cancelled run, and the whole of what + # a cancelled run is allowed to do to the trigger comment: take this bot's own + # reactions off it and say nothing. + # + # A newer `@seidroid review` cancels the run in flight. The eyes are already on + # the older comment by then, and the newer run answers its OWN comment id, so + # nothing reads the older one again -- it would wear the eyes for good. + # + # THIS STEP HAS NO POST. It withdraws, and there is no code path in it that adds + # a reaction, so nothing it is told can make it state an outcome. That is what + # makes the withdrawal a step of its own rather than always() on `Answer the + # request`: that step chooses a reaction from a conclusion, and a cancellation + # arriving after the driver finishes leaves check_path and verdict_produced + # populated -- so it would read a real conclusion and thumb a review that + # published nothing. + # + # `Post the verdict` decides whether there is anything to withdraw, and the + # driver does not. A cancellation can arrive once the verdict is already on the + # pull request -- during thread resolution, say -- and the thumb `Answer the + # request` posted answers it correctly. Withdrawing it there leaves a published + # review with no reaction on the request that asked for it, which reads as never + # answered. That is this step's own defect, one window later. + # + # Its `posted` OUTPUT separates the two, and its outcome does not. That step + # tolerates a failed comment POST and ends on a call whose failure it swallows, + # so it exits 0 and reads success whether the verdict landed or not. A thumb kept + # on that reading would stand for a review nobody can see. The output is written + # from the POST's own result. + # + # One boolean about another step, with no conclusion in it. It says whether an + # answer already stands, never which answer it would be, so reading it cannot + # give this step an outcome to state. The same holds for the answer step's + # outcome, which the withdrawal set below reads. + # + # Anything but a posted verdict withdraws. A value this step cannot read + # therefore clears, rather than leaving a thumb that stands for a verdict which + # may not be on the pull request. + # + # WHAT IT PROTECTS, AND WHERE THAT STOPS. A thumb survives when the verdict it + # answers is on the pull request AND this run is the run that posted it, or when + # this run never reached the answer step at all. One case is left over: this run + # answers, which withdraws an earlier run's thumb and posts its own, and is then + # cancelled before publishing. The comment ends bare while the earlier run's + # verdict still stands. The earlier thumb is already gone by then -- the answer + # step took it -- so nothing here can put it back, and knowing it happened would + # take a read of the pull request this step deliberately does not make. A + # cancellation lands during the driver far more often than in that window. + # + # LAST in the job, deliberately. The runner evaluates a step's condition when it + # reaches the step, so a clear placed earlier is already skipped once a + # cancellation lands on a later one -- and it would leave both the eyes and the + # thumb the step above had just posted. + # + # cancelled() is only readable in a step or job `if`. It is not available in + # `env` or `run`, and a workflow carrying it there is rejected, so the job status + # cannot be handed to a shell and this decision cannot move inside one script. + if: ${{ inputs.mode == 'review' && cancelled() + && needs.guard.outputs.comment_id != '' + && steps.verdict.outputs.posted != 'true' }} + continue-on-error: true + shell: bash + env: + GH_TOKEN: ${{ github.token }} + TRIGGER_REPO: ${{ github.repository }} + TRIGGER_ID: ${{ needs.guard.outputs.comment_id }} + # The collection the two steps above reacted on. It differs per event, so + # reading a fixed one would leave a diff-thread request wearing the eyes + # nothing here could reach. + COMMENT_API: ${{ needs.guard.outputs.comment_api }} + # Whether this run ever reached the step that answers. See the withdrawal set + # below. Four words about another step; none of them a conclusion. + ANSWERED: ${{ steps.answer.outcome }} + run: | + set -euo pipefail + # Scoped to the reacting identity, for the reason `Answer the request` states: + # a human who thumbed the request is voicing an opinion, and a cancellation is + # no licence to delete it. GITHUB_TOKEN reacts as github-actions[bot], and an + # installation token cannot ask the API which login it carries, so the login is + # named here. + me="github-actions[bot]" + # WHAT THIS RUN MAY TAKE, and it is not always all three. + # + # A re-run replays the trigger comment id, so the comment can already carry a + # thumb from an earlier run whose verdict IS on the pull request. `Answer the + # request` withdraws that thumb and posts this run's own, so once it has run, + # every reaction on the comment belongs to this run and this run published + # nothing -- take all three. + # + # A run cancelled before it reached that step has posted only the eyes. A thumb + # there answers an earlier run, and taking it leaves that run's published + # verdict with no reaction on the request, which reads as never answered. + # + # skipped is the only value that means "never touched the comment". failure and + # cancelled both mean the step ran partway and most likely cleared the earlier + # thumb already, so they clear, and so does a value this step cannot read: a + # thumb this run posted for a verdict nobody published is the worse of the two + # wrongs. + case "${ANSWERED:-}" in + skipped) takeable=" eyes " ;; + *) takeable=" +1 -1 eyes " ;; + esac + # Listed into a variable and read from it rather than through a pipe, so + # nothing this block reports can be read back as a reaction id. Paginated, + # because a busy comment carries more reactions than one page holds. + if ! mine="$(gh api "repos/$TRIGGER_REPO/$COMMENT_API/$TRIGGER_ID/reactions" \ + --paginate \ + --jq ".[] | select(.user.login == \"$me\") | \"\\(.content) \\(.id)\"")"; then + echo "::warning::could not read the reactions on comment $TRIGGER_ID in $TRIGGER_REPO; the eyes from this cancelled run may stay on it" + mine="" + fi + # Only what this run may take. A reaction of this bot's outside that set + # belongs to whatever put it there. + while read -r content rid; do + [ -n "$rid" ] || continue + case "$takeable" in *" $content "*) ;; *) continue ;; esac + if gh api -X DELETE \ + "repos/$TRIGGER_REPO/$COMMENT_API/$TRIGGER_ID/reactions/$rid" \ + >/dev/null; then + echo "withdrew this bot's $content from comment $TRIGGER_ID" + else + echo "::warning::could not withdraw this bot's $content from comment $TRIGGER_ID in $TRIGGER_REPO" + fi + done <<< "$mine"