diff --git a/.github/workflows/seidroid-review.yml b/.github/workflows/seidroid-review.yml index 860e6a6..f1b199e 100644 --- a/.github/workflows/seidroid-review.yml +++ b/.github/workflows/seidroid-review.yml @@ -11,13 +11,17 @@ run-name: UCI / seidroid review / ${{ (github.event.issue.number || github.event # check runs. # # TWO PATHS, one review. An AUTOMATIC review runs on `pull_request` when the caller -# wires that trigger and passes `mode: review`. A MANUAL one runs when a person -# comments `@seidroid review`. Both spend model quota and hold a sandbox, so both are -# gated -- see the guard below. Both paths refuse a fork-originated pull request. The -# automatic path reviews a pull request once. It reviews a later push only where the -# caller sets `re-review-on-push`, or where a block of its own stands. It also refuses -# a draft and honours the skip-review label. The manual path additionally checks who -# is asking. +# wires that trigger and passes `mode: review`. A MANUAL one runs when a person writes +# `@seidroid review` on the pull request: on the conversation, in a diff thread, or in +# a review body. Both spend model quota and hold a sandbox, so both are gated -- see +# the guard below. Both paths refuse a fork-originated pull request. The automatic path +# reviews a pull request once. It reviews a later push only where the caller sets +# `re-review-on-push`, or where a block of its own stands. It also refuses a draft and +# honours the skip-review label. The manual path additionally checks who is asking. +# +# FOUR EVENTS reach this file, and the guard refuses any other by name: `pull_request` +# for the automatic path, and `issue_comment`, `pull_request_review_comment` and +# `pull_request_review` for the manual one. A caller wires the ones it wants. # # This file is the automation of record. It REPLACES `ai-review.yml` rather than # running beside it; a repository that wires the automatic path here should retire @@ -107,6 +111,30 @@ on: required: false type: string default: 'v0.15.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 @@ -128,6 +156,32 @@ on: 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 @@ -374,12 +428,12 @@ on: organisation. Fork code sits outside that acceptance, and the guard refuses it. An - explicit `@seidroid review` arrives as an issue_comment in the base - repository, which carries the secrets. That path reaches a fork's code - unless something stops it. The guard's fork check is what stops it, on - that path and on the automatic one. This shell therefore runs only over - code from inside the organisation. Weigh that before you widen or narrow - this list. + 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' @@ -502,12 +556,45 @@ jobs: 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, and for any comment-triggered - # dispatch, review or close. For a comment it decides whether the commenter 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. + # 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 @@ -515,27 +602,91 @@ jobs: # organisation, on this path as well as the comment path. if: >- ${{ (github.event_name == 'pull_request' && inputs.mode == 'review') || - (github.event_name == 'issue_comment' && - github.event.issue.pull_request != null && - github.event.comment.user.type != 'Bot' && - contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association)) }} + (((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 }} + run: | + set -uo pipefail + 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 comment body, the PR number, the + # 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 }} + 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 }} + 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 @@ -547,20 +698,60 @@ jobs: 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 so `@seidroid review` -- the documented form, and what - # the mention actually notifies -- and a bare `seidroid review` both work. - # Whole-line anchoring is what keeps that safe: a comment discussing the - # command has other words on the line and does not match. + # 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:]]*@?seidroid[[:space:]]+review([[:space:]]+close)?[[:space:]]*$' || true)" + | 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 @@ -587,13 +778,16 @@ jobs: # 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:]]*@?seidroid[[: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)" + | 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" @@ -601,7 +795,18 @@ jobs: # 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. - echo "comment_id=$COMMENT_ID" + # + # 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 @@ -623,8 +828,13 @@ jobs: env: GH_TOKEN: ${{ steps.identity.outputs.token }} ALLOWED_TEAM: ${{ inputs.allowed-team }} + ALLOWED_BOTS: ${{ inputs.allowed-bots }} SKIP_LABEL: ${{ inputs.skip-review-label }} - ACTOR: ${{ github.event.comment.user.login }} + # 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 }} @@ -664,32 +874,61 @@ jobs: deny "$REPO#$PR is a draft; not reviewing" fi - # Membership is a security control, so it fails closed: empty, malformed - # and unanswerable all deny. The job condition has already required an - # OWNER/MEMBER/COLLABORATOR association, which admits any collaborator on - # the repository the request was made in; a team narrows that. + # 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. # - # It gates who may COMMAND a review, so it applies to the comment path only. - # An automatic run has no commander: applying the team check there would - # silently stop reviewing every pull request opened by anyone outside the - # team, which is the opposite of what a caller sets this input for. + # 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". # - # It stops a REVIEW, not a teardown, for the reason the label check below - # states. Any collaborator the job condition admits may reclaim a sandbox, - # whether or not they are on the team, because the alternative is a sandbox - # nothing reclaims. + # 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. # - # 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. + # 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 - 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" + # 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 @@ -703,9 +942,17 @@ jobs: # repository can turn that withholding off, per repository or by organisation # policy. This check does not rest on a setting nobody here controls. # - # A pull_request payload carries both repository ids, so that path spends no - # API call. An issue_comment payload carries no head repository, so the API - # answers there. Repository ids, not names, so a rename does not read as a + # 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" @@ -934,9 +1181,15 @@ jobs: # 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') || @@ -976,11 +1229,17 @@ jobs: 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. A reaction on a PR comment goes to the - # ISSUE comments endpoint, which pull-requests: write does not cover. GitHub's - # permission table grants that endpoint's POST and DELETE to Issues alone, and - # grants the comment itself to either scope, so the alias stops at the reaction. - # GraphQL addReaction is the other route to it, and GitHub documents no + # 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 @@ -1024,6 +1283,10 @@ jobs: 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 @@ -1032,8 +1295,9 @@ jobs: # # `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. - if gh api -X POST "repos/$REPO/issues/comments/$TRIGGER_ID/reactions" \ + # 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 @@ -1498,7 +1762,11 @@ jobs: # 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: ${{ needs.guard.outputs.comment_id }} + # 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 @@ -2431,6 +2699,9 @@ jobs: # 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 }} @@ -2491,7 +2762,7 @@ jobs: # 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/issues/comments/$TRIGGER_ID/reactions" \ + 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" @@ -2501,7 +2772,7 @@ jobs: [ -n "$rid" ] || continue case " $stale " in *" $content "*) ;; *) continue ;; esac if gh api -X DELETE \ - "repos/$TRIGGER_REPO/issues/comments/$TRIGGER_ID/reactions/$rid" \ + "repos/$TRIGGER_REPO/$COMMENT_API/$TRIGGER_ID/reactions/$rid" \ >/dev/null; then echo "withdrew this bot's $content from comment $TRIGGER_ID" else @@ -2516,7 +2787,7 @@ jobs: # 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/issues/comments/$TRIGGER_ID/reactions" \ + 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 @@ -3254,6 +3525,10 @@ jobs: 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 }} @@ -3289,7 +3564,7 @@ jobs: # 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/issues/comments/$TRIGGER_ID/reactions" \ + 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" @@ -3301,7 +3576,7 @@ jobs: [ -n "$rid" ] || continue case "$takeable" in *" $content "*) ;; *) continue ;; esac if gh api -X DELETE \ - "repos/$TRIGGER_REPO/issues/comments/$TRIGGER_ID/reactions/$rid" \ + "repos/$TRIGGER_REPO/$COMMENT_API/$TRIGGER_ID/reactions/$rid" \ >/dev/null; then echo "withdrew this bot's $content from comment $TRIGGER_ID" else diff --git a/.github/workflows/workflow-test-self.yml b/.github/workflows/workflow-test-self.yml index 46b8191..07e7da3 100644 --- a/.github/workflows/workflow-test-self.yml +++ b/.github/workflows/workflow-test-self.yml @@ -1,20 +1,38 @@ name: Workflow tests -# The shell and jq inside seidroid-review.yml, run against a gh stub. Nothing here -# reaches the GitHub API, so this needs no token and no permissions. +# The shell and jq inside seidroid-review.yml, run against a gh stub, and its job +# conditions evaluated against synthetic payloads. Nothing here reaches the GitHub +# API, so this needs no token and no permissions. # -# Two steps, one harness. Placement records which thread each posted comment -# replaced and the resolve step closes on that record, so the pair is the behaviour -# worth testing rather than either half. +# Three jobs, so each check in the list names the step it covers. A job that ran two +# harnesses would report one name for two things, and one check would go red for the +# other's failure. +# +# `Place findings and resolve threads` covers a pair of steps rather than either half: +# placement records which thread each posted comment replaced, and the resolve step +# closes on that record. `The reaction steps` covers which of the three runs in which +# job state, and what each leaves on the trigger comment. `Guard the request` covers +# the admission path, from the event the workflow accepts to the collection the +# acknowledgement reaches. on: pull_request: paths: - '.github/workflows/seidroid-review.yml' + # run-guard.sh asserts what ai-assistant.yml claims of the same comment, so an + # edit there can break an invariant stated here. Without this the break lands + # on the next unrelated pull request that touches the file above, pointing at + # the wrong change. + - '.github/workflows/ai-assistant.yml' - '.github/workflows/workflow-test-self.yml' - 'test/seidroid-review/**' push: branches: [ main ] paths: - '.github/workflows/seidroid-review.yml' + # run-guard.sh asserts what ai-assistant.yml claims of the same comment, so an + # edit there can break an invariant stated here. Without this the break lands + # on the next unrelated pull request that touches the file above, pointing at + # the wrong change. + - '.github/workflows/ai-assistant.yml' - '.github/workflows/workflow-test-self.yml' - 'test/seidroid-review/**' permissions: @@ -35,9 +53,6 @@ jobs: - name: Run the placement and resolve harness run: test/seidroid-review/run.sh - # Its own job, so each check in the list names the step it covers. A job that runs - # both harnesses would report one name for two things, and the placement check would - # go red for a reaction. reactions: name: The reaction steps runs-on: ubuntu-latest @@ -56,3 +71,18 @@ jobs: # lives and which the shell harness above cannot see. - name: Check the step conditions run: python3 test/seidroid-review/conditions.py .github/workflows/seidroid-review.yml + + guard-admission: + name: Guard the request + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v7 + - name: Set up Python + uses: actions/setup-python@v7 + with: + python-version: '3.x' + - name: Install the YAML reader + run: python3 -m pip install --quiet pyyaml + - name: Run the guard harness + run: test/seidroid-review/run-guard.sh diff --git a/test/seidroid-review/.gitignore b/test/seidroid-review/.gitignore index c43180d..123dcfa 100644 --- a/test/seidroid-review/.gitignore +++ b/test/seidroid-review/.gitignore @@ -9,3 +9,11 @@ ack.sh answer.sh withdraw.sh out-reactions/ +# Written by run-guard.sh: the five steps it extracts from the workflow. Two of +# them are steps reactions.sh also extracts, under names of their own so neither +# harness can overwrite the other's extraction. +refuse.sh +parse.sh +admit.sh +guard-ack.sh +guard-answer.sh diff --git a/test/seidroid-review/README.md b/test/seidroid-review/README.md index 734df8c..17b1c6b 100644 --- a/test/seidroid-review/README.md +++ b/test/seidroid-review/README.md @@ -1,40 +1,48 @@ # Workflow tests -Two harnesses over `.github/workflows/seidroid-review.yml`. Both read the steps out -of the YAML on every run, so neither can pass against a stale copy. +Three harnesses over `.github/workflows/seidroid-review.yml`. All three read the +steps out of the YAML on every run, so none can pass against a stale copy. ```sh test/seidroid-review/run.sh # placement and thread resolution test/seidroid-review/reactions.sh # the three reaction steps +test/seidroid-review/run-guard.sh # the guard, and the reaction collection python3 test/seidroid-review/conditions.py .github/workflows/seidroid-review.yml ``` -# `Place findings on the code` and `Resolve the threads this review closed` +Each needs `bash`, `jq`, and `python3` with PyYAML, and exits non-zero on the +first failed assertion count. -Runs both steps under `bash`, against a `gh` stub, and checks what they posted, -counted and closed. +`extract.py` is shared. It reads a step's `run:` block and a workflow-level env +key out of the YAML, by step name or step id. + +`reactions.sh` and `run-guard.sh` both extract `Acknowledge the trigger` and +`Answer the request`, and they ask different things of them: `reactions.sh` asks +which step runs in which job state and what it leaves on the comment, +`run-guard.sh` asks which REST collection the URL reaches. Each writes its own +extraction — `ack.sh` and `answer.sh` against `guard-ack.sh` and +`guard-answer.sh` — so running both cannot have one overwrite the other. -The run needs `bash`, `jq`, and `python3` with PyYAML. It exits non-zero on the -first failed assertion count and prints a table of one row per case. +## `Place findings on the code` and `Resolve the threads this review closed` + +Runs both steps under `bash`, against a `gh` stub, and checks what they posted, +counted and closed. It prints a table of one row per case. Both steps are in one harness because they are one behaviour. Placement records which thread each posted comment replaced; the resolve step closes a thread on finding its id in that record. A harness that ran only one of them could not tell whether the record it wrote is the record the other reads. -## How it works - -`extract.py` reads a step's `run:` block and the workflow's `FINDING_MARKER` out -of the YAML on every run, so the harness tests the file as it stands. It runs -twice, once per step, and the two markers are asserted equal: placement stamps a -comment with it and the resolve step recognises a thread by it. +The extractor runs twice, once per step, and the two `FINDING_MARKER` readings +are asserted equal: placement stamps a comment with it and the resolve step +recognises a thread by it. `bin/gh` goes on `PATH` ahead of the real `gh`. It logs every call, serves fixture JSON through the step's own `jq`, keeps the request body the step sent, and decides per case whether a call succeeds. `STUB_*` variables in `run_case` and `run_resolve` select the fixtures and the answers. -## The fixtures +### The fixtures `fx/files*.json` are `GET /compare` responses. One JSON object each: compare paginates its commits, and a second page carries no `files` key, so the step @@ -59,7 +67,7 @@ because every body has to open with the marker the workflow defines now: two pages, and four threads that fail this step's own tests — the other identity, a foreign account, no marker, and a marker quoted mid-body. -# The reaction steps +## The reaction steps `reactions.sh` runs `Acknowledge the trigger`, `Answer the request` and `Withdraw the reactions on a cancelled run` against `bin-reactions/gh`, which keeps @@ -89,7 +97,7 @@ later cancellation. And a run cancelled before it reached `Answer the request` t only the eyes: a thumb on the comment then belongs to an EARLIER run, whose verdict may still stand. -# The step conditions +## The step conditions `conditions.py` covers what a shell harness cannot see. A step condition decides which reaction step runs in which job state, and that is where the cancellation behaviour @@ -118,3 +126,65 @@ step is not invisible to them, and both search the **whole step** rather than on No check needs telling where to look. A check that has to be pointed at a step is not stated over the file. + +## The guard, and the two steps that react + +Runs the request-admission path under `bash` against a `gh` stub of its own, and +evaluates the shipped job conditions against synthetic event payloads. Five +steps are read: `Refuse an event this workflow does not handle`, `parse`, `Admit +the request`, `Acknowledge the trigger` and `Answer the request`. + +`gha.py` covers what a script harness cannot see. A job condition and a step's +`env:` mapping are GitHub expressions, and both decide which payload field a +request is read from, so both are evaluated here rather than restated. It models +four GitHub semantics the conditions rest on — case-insensitive string +comparison, `||` and `&&` yielding one operand each, **both short-circuiting**, +and `contains` over an array testing membership — and `--selftest` checks each +one. That model is read from GitHub's published expression semantics: nothing in +this directory calls a runner. + +Short-circuiting is the one to be careful with. The runner's Or and And nodes +return on the first truthy or falsy operand and never evaluate the rest, so a +`fromJSON` an operand nothing reaches would refuse never runs. A model that +evaluated eagerly reports a failure the runner does not have, and one assertion +here stated the opposite of what a real event does before this was modelled. + +Four modes: + +```sh +gha.py # the job's if:, as true or false +gha.py --env # what a step's env key resolves to +gha.py --input # a declared workflow_call input field +gha.py --selftest # the expression model itself +``` + +One group reaches outside this file. `ai-assistant.yml` answers the same comments +and reserves the exact `@seidroid review` body for the reviewer, so the last group +evaluates that workflow's own reply condition beside the parse and records which +tool answers each body. Every body is checked on all three comment events, because +the assistant has a branch each and this workflow now answers all three: a helper +naming one event would measure the division on the path that already had it and +infer the two this workflow adds. Two bodies both tools answer; the group says +which and why. + +`ai-assistant.yml` is in `workflow-test-self.yml`'s `paths:` filter for that +reason. Without it an edit there breaks an invariant stated here, and the break +lands on the next unrelated pull request that touches `seidroid-review.yml`. + +`bin-guard/gh` logs every call, serves the answer the case chose through the +step's own `--jq` filter, and tells the fork check from the label check by the +filter each sends. A failed read prints nothing and exits non-zero, which is the +shape `Admit the request` is written against: it captures stdout, so an empty +capture is what tells the fork check and the once-per-PR gate that nobody +answered. `bin/gh` beside it files an error body instead, because the placement +step reads one. + +### The fixtures + +There are none. A guard case turns on six payload fields and five API answers, +so each is built in the run from `STUB_*` and context arguments, where the case +that chose it can be read beside the assertion it drives. + +`STUB_TEAM`, `STUB_ORIGIN`, `STUB_LABELS`, `STUB_REVIEWS`, `STUB_COMMENTS` and +`STUB_REACTIONS` choose what the stub answers; `FAIL` on any of them is a read +that nobody answered. \ No newline at end of file diff --git a/test/seidroid-review/bin-guard/gh b/test/seidroid-review/bin-guard/gh new file mode 100755 index 0000000..66c4020 --- /dev/null +++ b/test/seidroid-review/bin-guard/gh @@ -0,0 +1,115 @@ +#!/usr/bin/env bash +# gh stub for the guard steps: logs every call, serves the answer the case chose, +# and runs the step's own --jq filter over it. +# +# A failed read prints nothing and exits non-zero, which is the shape `Admit the +# request` is written against: it captures stdout, so an empty capture is what +# tells the fork check and the once-per-PR gate that nobody answered. The +# placement stub beside this one files an error body instead, because that step +# reads one. +# +# Five reads reach here. The team membership read and the once-per-PR reads are +# told apart by their path; the fork check and the label check share a path and +# are told apart by the filter each sends. +log() { printf '%s\n' "$*" >> "$STUB_LOG"; } + +argv=("$@") +joined="$*" + +filter="" +n=${#argv[@]} +for ((i = 0; i < n; i++)); do + [ "${argv[i]}" = "--jq" ] && filter="${argv[i + 1]}" +done + +serve() { # fixture-json + printf '%s' "$1" | jq -r "$filter" + exit 0 +} + +refuse() { # what-failed + log "CALL $1 FAILED" + echo "gh: the stub was told this read fails" >&2 + exit 1 +} + +# Ahead of the comment reads below: an issue comment's reactions path carries +# /issues/ and comments too, and the first matching case wins. +case "$joined" in + *"/reactions"*) + method=GET + for ((i = 0; i < n; i++)); do + [ "${argv[i]}" = "-X" ] && method="${argv[i + 1]}" + done + for a in "${argv[@]}"; do case "$a" in repos/*/reactions*) url="$a" ;; esac; done + log "CALL reaction $method ${url:-none}" + [ "${STUB_REACTIONS:-none}" = "FAIL" ] && refuse reaction + case "$method" in + GET) + case "${STUB_REACTIONS:-none}" in + eyes) serve '[{"id":1,"content":"eyes","user":{"login":"github-actions[bot]"}}]' ;; + *) serve '[]' ;; + esac + ;; + *) exit 0 ;; + esac + ;; +esac + +case "$joined" in + *"/teams/"*"/memberships/"*) + for a in "${argv[@]}"; do case "$a" in */memberships/*) who="${a##*/}" ;; esac; done + log "CALL membership $who" + [ "${STUB_TEAM:-active}" = "FAIL" ] && refuse membership + serve "{\"state\":\"${STUB_TEAM:-active}\"}" + ;; +esac + +case "$joined" in + *"/pulls/"*"/reviews"*) + log "CALL reviews" + [ "${STUB_REVIEWS:-none}" = "FAIL" ] && refuse reviews + case "${STUB_REVIEWS:-none}" in + blocked) serve "[{\"id\":11,\"state\":\"CHANGES_REQUESTED\",\"body\":\"${VERDICT_MARKER}a block\"}]" ;; + *) serve '[]' ;; + esac + ;; +esac + +case "$joined" in + *"/issues/"*"/comments"*) + log "CALL comments" + [ "${STUB_COMMENTS:-none}" = "FAIL" ] && refuse comments + case "${STUB_COMMENTS:-none}" in + verdict) serve "[{\"id\":22,\"user\":{\"type\":\"Bot\"},\"body\":\"${VERDICT_MARKER}a verdict\"}]" ;; + *) serve '[]' ;; + esac + ;; +esac + +case "$joined" in + *"/pulls/"*) + # The fork check names head.repo.id in its filter; the label check names labels. + case "$filter" in + *head.repo.id*) + log "CALL origin" + [ "${STUB_ORIGIN:-same}" = "FAIL" ] && refuse origin + case "${STUB_ORIGIN:-same}" in + same) serve '{"head":{"repo":{"id":1}},"base":{"repo":{"id":1}}}' ;; + fork) serve '{"head":{"repo":{"id":2}},"base":{"repo":{"id":1}}}' ;; + null) serve '{"head":{"repo":null},"base":{"repo":{"id":1}}}' ;; + esac + ;; + *labels*) + log "CALL labels" + [ "${STUB_LABELS:-}" = "FAIL" ] && refuse labels + names="$(jq -nc --arg list "${STUB_LABELS:-}" \ + 'if $list == "" then [] else ($list | split(",") | map({name: .})) end')" + serve "{\"labels\":$names}" + ;; + esac + ;; +esac + +log "CALL unhandled $joined" +exit 1 diff --git a/test/seidroid-review/bin-reactions/gh b/test/seidroid-review/bin-reactions/gh index 6cd9107..001122a 100755 --- a/test/seidroid-review/bin-reactions/gh +++ b/test/seidroid-review/bin-reactions/gh @@ -27,7 +27,7 @@ done case "$verb:$path" in GET:*/reactions) - log "CALL list" + log "CALL list $path" [ "${STUB_LIST:-ok}" = FAIL ] && exit 1 # Through the step's own filter, so the filter is under test and not the harness's # idea of it. @@ -35,7 +35,7 @@ case "$verb:$path" in ;; DELETE:*/reactions/*) rid="${path##*/}" - log "CALL delete $rid" + log "CALL delete $rid $path" [ "${STUB_DELETE:-ok}" = FAIL ] && exit 1 jq -e --argjson rid "$rid" 'any(.[]; .id == $rid)' < "$STUB_STATE" > /dev/null || { log " no such reaction $rid"; exit 1; } @@ -45,7 +45,7 @@ case "$verb:$path" in POST:*/reactions) content="" for f in "${fields[@]:-}"; do case "$f" in content=*) content="${f#content=}" ;; esac; done - log "CALL post $content" + log "CALL post $content $path" [ "${STUB_POST:-ok}" = FAIL ] && exit 1 # Idempotent per (user, content): the API returns the reaction already there # rather than adding a second one. diff --git a/test/seidroid-review/extract.py b/test/seidroid-review/extract.py index 249b862..ad804ff 100644 --- a/test/seidroid-review/extract.py +++ b/test/seidroid-review/extract.py @@ -1,10 +1,11 @@ import sys, yaml path, step, out = sys.argv[1], sys.argv[2], sys.argv[3] +key = sys.argv[4] if len(sys.argv) > 4 else "FINDING_MARKER" d = yaml.safe_load(open(path, encoding="utf-8")) for job in d["jobs"].values(): for s in job.get("steps", []): - if s.get("name") == step: + if s.get("name") == step or s.get("id") == step: open(out, "w", encoding="utf-8").write(s["run"]) - print(d["env"]["FINDING_MARKER"]) + print(d["env"][key]) sys.exit(0) sys.exit("step not found: " + step) diff --git a/test/seidroid-review/gha.py b/test/seidroid-review/gha.py new file mode 100644 index 0000000..f9f85d5 --- /dev/null +++ b/test/seidroid-review/gha.py @@ -0,0 +1,402 @@ +"""Evaluates a job's `if:` expression out of a workflow file. + +The subset covered is the one seidroid-review.yml's own conditions use: the +operators `!`, `==`, `!=`, `<`, `<=`, `>`, `>=`, `&&` and `||`; the functions +`contains`, `fromJSON`, `startsWith`, `endsWith`, `format`, `join`, `toJSON`, +`always`, `cancelled`, `success` and `failure`; and dotted lookups into the +`github`, `inputs`, `needs`, `steps`, `env` and `secrets` contexts. + +Four GitHub semantics carry the conditions under test, so they are modelled +exactly and `--selftest` checks each one: + + * `==` on two strings ignores case, and on mixed types casts both to number, + where null and the empty string are 0 and any other non-numeric string is + NaN. NaN equals nothing. + * `a || b` yields `a` when `a` is truthy and `b` otherwise. False, 0, the + empty string and null are the falsy values. `a && b` yields `a` when `a` is + falsy and `b` otherwise. + * BOTH SHORT-CIRCUIT. The runner's Or and And nodes return on the first + truthy or falsy operand and never evaluate the rest, so an error in an + operand nothing reaches never surfaces. A model that evaluated eagerly would + report a failure the runner does not have -- and would make an assertion here + state the opposite of what a real event does. + * `contains(array, item)` tests membership under that same loose equality, + so a listed login matches whatever case it is written in. `contains` over a + STRING tests substring instead, which is the reading this file's conditions + must not have. + +The fidelity of that model rests on GitHub's published expression semantics, +read rather than measured: nothing here calls a runner. + +Usage: + gha.py -> prints true or false + gha.py --expr '' -> prints true or false + gha.py --env -> prints the value + gha.py --input -> prints a declared field + gha.py --selftest -> checks the model above +""" + +import json +import math +import re +import sys + +import yaml + +NAN = float("nan") + +TOKEN = re.compile( + r"""\s+ + |(?P'(?:[^']|'')*') + |(?P\d+(?:\.\d+)?(?:[eE][-+]?\d+)?) + |(?P==|!=|>=|<=|&&|\|\||[!<>(),.\[\]]) + |(?P[A-Za-z_][A-Za-z0-9_-]*)""", + re.X, +) + + +class Bad(Exception): + """An expression GitHub would refuse, such as fromJSON over a non-JSON input.""" + + +def lex(text): + tokens, i = [], 0 + while i < len(text): + m = TOKEN.match(text, i) + if not m: + raise Bad("cannot read expression at: " + text[i:i + 20]) + i = m.end() + for kind in ("str", "num", "op", "name"): + if m.group(kind) is not None: + tokens.append((kind, m.group(kind))) + break + tokens.append(("end", "")) + return tokens + + +class Parser: + def __init__(self, tokens, ctx): + self.t, self.i, self.ctx = tokens, 0, ctx + # False while walking an operand the runner would not reach. The tokens still + # have to be consumed -- this evaluates as it parses -- so the walk continues + # and only the function calls are held back, which is where an error lives. + self.live = True + + def skip(self, parse): + was, self.live = self.live, False + try: + parse() + finally: + self.live = was + + def peek(self): + return self.t[self.i] + + def take(self, value=None): + kind, text = self.t[self.i] + if value is not None and text != value: + raise Bad("expected %r, found %r" % (value, text)) + self.i += 1 + return text + + def parse(self): + value = self.or_() + if self.peek()[0] != "end": + raise Bad("trailing input at %r" % (self.peek()[1],)) + return value + + def or_(self): + left = self.and_() + while self.peek()[1] == "||": + self.take() + if truthy(left): + self.skip(self.and_) + else: + left = self.and_() + return left + + def and_(self): + left = self.compare() + while self.peek()[1] == "&&": + self.take() + if truthy(left): + left = self.compare() + else: + self.skip(self.compare) + return left + + def compare(self): + left = self.unary() + while self.peek()[1] in ("==", "!=", "<", "<=", ">", ">="): + op = self.take() + right = self.unary() + if op == "==": + left = loose_eq(left, right) + elif op == "!=": + left = not loose_eq(left, right) + else: + a, b = to_number(left), to_number(right) + if math.isnan(a) or math.isnan(b): + left = False + else: + left = {"<": a < b, "<=": a <= b, ">": a > b, ">=": a >= b}[op] + return left + + def unary(self): + if self.peek()[1] == "!": + self.take() + return not truthy(self.unary()) + return self.primary() + + def primary(self): + kind, text = self.peek() + if text == "(": + self.take() + value = self.or_() + self.take(")") + return value + if kind == "str": + self.take() + return text[1:-1].replace("''", "'") + if kind == "num": + self.take() + return float(text) + if kind != "name": + raise Bad("unexpected %r" % (text,)) + self.take() + if text == "true": + return True + if text == "false": + return False + if text == "null": + return None + if self.peek()[1] == "(": + return self.call(text) + return self.path(self.ctx.get(text, {})) + + def path(self, value): + while self.peek()[1] == ".": + self.take() + key = self.take() + value = value.get(key) if isinstance(value, dict) else None + return value + + def call(self, name): + self.take("(") + args = [] + if self.peek()[1] != ")": + args.append(self.or_()) + while self.peek()[1] == ",": + self.take() + args.append(self.or_()) + self.take(")") + # An unreached call is not made, which is the whole of short-circuiting: this + # is where fromJSON would refuse a value the runner never looks at. + if not self.live: + return None + return apply_function(name, args) + + +def apply_function(name, args): + if name == "always": + return True + if name == "success": + return True + if name in ("cancelled", "failure"): + return False + if name == "contains": + return gha_contains(args[0], args[1]) + if name == "startsWith": + return as_string(args[0]).lower().startswith(as_string(args[1]).lower()) + if name == "endsWith": + return as_string(args[0]).lower().endswith(as_string(args[1]).lower()) + if name == "fromJSON": + try: + return json.loads(as_string(args[0])) + except ValueError as err: + raise Bad("fromJSON: %s" % (err,)) + if name == "toJSON": + return json.dumps(args[0]) + if name == "format": + out = as_string(args[0]) + for index, value in enumerate(args[1:]): + out = out.replace("{%d}" % index, as_string(value)) + return out + if name == "join": + sep = as_string(args[1]) if len(args) > 1 else "," + items = args[0] if isinstance(args[0], list) else [args[0]] + return sep.join(as_string(item) for item in items) + raise Bad("unsupported function: " + name) + + +def truthy(value): + if value is None or value is False: + return False + if value is True: + return True + if isinstance(value, (int, float)): + return not (value == 0 or math.isnan(value)) + if isinstance(value, str): + return value != "" + return True + + +def to_number(value): + if value is None: + return 0.0 + if isinstance(value, bool): + return 1.0 if value else 0.0 + if isinstance(value, (int, float)): + return float(value) + if isinstance(value, str): + text = value.strip() + if text == "": + return 0.0 + try: + return float(int(text, 16)) if text[:2].lower() == "0x" else float(text) + except ValueError: + return NAN + return NAN + + +def as_string(value): + if value is None: + return "" + if value is True: + return "true" + if value is False: + return "false" + if isinstance(value, float) and value.is_integer(): + return str(int(value)) + if isinstance(value, (list, dict)): + return json.dumps(value) + return str(value) + + +def loose_eq(a, b): + if isinstance(a, str) and isinstance(b, str): + return a.lower() == b.lower() + if isinstance(a, bool) and isinstance(b, bool): + return a is b + if a is None and b is None: + return True + if isinstance(a, (list, dict)) or isinstance(b, (list, dict)): + return a is b + x, y = to_number(a), to_number(b) + if math.isnan(x) or math.isnan(y): + return False + return x == y + + +def gha_contains(search, item): + if isinstance(search, list): + return any(loose_eq(element, item) for element in search) + if isinstance(search, dict): + return False + return as_string(item).lower() in as_string(search).lower() + + +def evaluate(text, ctx): + text = text.strip() + if text.startswith("${{") and text.endswith("}}"): + text = text[3:-2] + return Parser(lex(text), ctx).parse() + + +def job_condition(workflow, job_id): + doc = yaml.safe_load(open(workflow, encoding="utf-8")) + job = doc["jobs"][job_id] + if "if" not in job: + sys.exit("job has no if: " + job_id) + return job["if"] + + +def input_field(workflow, name, field): + """One field of one workflow_call input, so a declared default is testable.""" + doc = yaml.safe_load(open(workflow, encoding="utf-8")) + # PyYAML reads the `on:` key as the boolean True. + triggers = doc.get("on", doc.get(True)) + spec = triggers["workflow_call"]["inputs"] + if name not in spec: + sys.exit("no such input: " + name) + return spec[name].get(field) + + +def step_env(workflow, step, key): + """The expression a step's env key holds, found by step name or step id. + + The step scripts read these as shell variables, so a script harness cannot + see them. They are where a per-event payload field is chosen. + """ + doc = yaml.safe_load(open(workflow, encoding="utf-8")) + for job in doc["jobs"].values(): + for candidate in job.get("steps", []): + if candidate.get("name") == step or candidate.get("id") == step: + env = candidate.get("env") or {} + if key not in env: + sys.exit("step %s has no env %s" % (step, key)) + return env[key] + sys.exit("step not found: " + step) + + +SELFTEST = [ + ("'Bot' == 'bot'", True), + ("'MEMBER' != 'member'", False), + ("null == ''", True), + ("null == 'Bot'", False), + ("'' != 'Bot'", True), + ("true == 1", True), + ("'' || 'second'", "second"), + ("'first' || 'second'", "first"), + ("null || ''", ""), + ("contains(fromJSON('[\"dependabot[bot]\"]'), 'DependaBot[BOT]')", True), + ("contains(fromJSON('[\"dependabot[bot]\"]'), 'bot')", False), + ("contains('[\"dependabot[bot]\"]', 'bot')", True), + ("contains(fromJSON('[]'), 'anyone')", False), + ("!contains(fromJSON('[\"a\",\"b\"]'), 'c')", True), + # Short-circuiting, stated as the three shapes the conditions rely on. + ("true || fromJSON('not json')", True), + ("false && fromJSON('not json')", False), + ("'' || '[]'", "[]"), + ("'[\"x\"]' || '[]'", '["x"]'), +] + + +def selftest(): + failed = 0 + for expression, want in SELFTEST: + got = evaluate(expression, {}) + if got != want: + failed += 1 + print(" FAIL %s: want %r got %r" % (expression, want, got)) + print("gha.py selftest: %d of %d checks passed" % (len(SELFTEST) - failed, len(SELFTEST))) + return 1 if failed else 0 + + +def main(argv): + if argv[1:2] == ["--selftest"]: + return selftest() + raw = False + if argv[1:2] == ["--expr"]: + text, ctx_path = argv[2], argv[3] + elif argv[1:2] == ["--input"]: + print(as_string(input_field(argv[2], argv[3], argv[4]))) + return 0 + elif argv[1:2] == ["--env"]: + # The value a step's env key resolves to, printed as the shell would see + # it rather than as a truth value. + text, ctx_path, raw = step_env(argv[2], argv[3], argv[4]), argv[5], True + else: + text, ctx_path = job_condition(argv[1], argv[2]), argv[3] + ctx = json.load(open(ctx_path, encoding="utf-8")) + try: + value = evaluate(text, ctx) + except Bad as err: + print("error: %s" % (err,), file=sys.stderr) + return 2 + print(as_string(value) if raw else ("true" if truthy(value) else "false")) + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv)) diff --git a/test/seidroid-review/reactions.sh b/test/seidroid-review/reactions.sh index 42b531f..fabc90d 100755 --- a/test/seidroid-review/reactions.sh +++ b/test/seidroid-review/reactions.sh @@ -81,6 +81,10 @@ run_case() { export STUB_LIST=ok STUB_DELETE=ok STUB_POST=ok STUB_ACTOR="$BOT" export PATH="$HERE/bin-reactions:$PATH" export GH_TOKEN=x REPO=owner/repo TRIGGER_REPO=owner/repo TRIGGER_ID=7 + # The collection the guard resolved for the object that asked. All three steps read + # it, and the last group below is the one that varies it; every case before that is + # about what a step leaves on the comment rather than where it reached. + export COMMENT_API=issues/comments local check_path="" if [ "$have_check" = yes ]; then printf '{"conclusion":"%s","title":"t"}\n' "$conclusion" > "$CASE/check.json" @@ -287,6 +291,47 @@ CASE="$HERE/out-reactions/close-mode"; rm -rf "$CASE"; mkdir -p "$CASE" check "close selects no step" "" "$($SELECT success close 7 - | paste -sd, -)" check "cancelled close selects no step" "" "$($SELECT cancelled close 7 false | paste -sd, -)" +# The trigger's reactions live under a different collection per event, so every step +# that touches them builds its URL from the guard's comment_api rather than naming one. +# A step that named one would post the eyes where the answer cannot reach them, or +# withdraw from a comment that never carried them. +echo "== every reaction reaches the collection the guard resolved ==" +run_case api-issue "$NONE" success success true success true yes +check "the answer read the issue collection" 1 \ + "$(grep -c '^CALL list repos/owner/repo/issues/comments/7/reactions$' "$CASE/calls.log")" +check "and thumbed there" 1 \ + "$(grep -c '^CALL post +1 repos/owner/repo/issues/comments/7/reactions$' "$CASE/calls.log")" +check "the acknowledgement too" 1 \ + "$(grep -c '^CALL post eyes repos/owner/repo/issues/comments/7/reactions$' "$CASE/ack-calls.log")" +run_case api-thread "$NONE" success success true success true yes COMMENT_API=pulls/comments +check "the answer read the pull collection" 1 \ + "$(grep -c '^CALL list repos/owner/repo/pulls/comments/7/reactions$' "$CASE/calls.log")" +check "and thumbed there" 1 \ + "$(grep -c '^CALL post +1 repos/owner/repo/pulls/comments/7/reactions$' "$CASE/calls.log")" +check "the acknowledgement too" 1 \ + "$(grep -c '^CALL post eyes repos/owner/repo/pulls/comments/7/reactions$' "$CASE/ack-calls.log")" +check "nothing reached the other one" 0 \ + "$(cat "$CASE/calls.log" "$CASE/ack-calls.log" | grep -c 'issues/comments')" +# The withdrawal step, which runs on no path the two cases above take. It only ever +# reads and deletes, so a wrong collection there is the case where the eyes stay. The +# acknowledgement seeds the eyes, and the id it lands is the stub's to choose, so the +# delete is matched on its collection rather than on that id. +run_case api-withdraw-issue "$NONE" cancelled cancelled false - '' no +check "the withdrawal read the issue collection" 1 \ + "$(grep -c '^CALL list repos/owner/repo/issues/comments/7/reactions$' "$CASE/calls.log")" +check "and deleted the eyes there" 1 \ + "$(grep -cE '^CALL delete [0-9]+ repos/owner/repo/issues/comments/7/reactions/[0-9]+$' "$CASE/calls.log")" +check "the comment is clear" "" "$(left)" +run_case api-withdraw-thread "$NONE" cancelled cancelled false - '' no COMMENT_API=pulls/comments +check "the withdrawal read the pull collection" 1 \ + "$(grep -c '^CALL list repos/owner/repo/pulls/comments/7/reactions$' "$CASE/calls.log")" +check "and deleted the eyes there" 1 \ + "$(grep -cE '^CALL delete [0-9]+ repos/owner/repo/pulls/comments/7/reactions/[0-9]+$' "$CASE/calls.log")" +check "the comment is clear" "" "$(left)" +check "nothing reached the other one" 0 \ + "$(cat "$CASE/calls.log" "$CASE/ack-calls.log" | grep -c 'issues/comments')" +check "and it still posts nothing" 0 "$(calls post)" + echo "== no step made a call the stub does not serve ==" check "unstubbed calls" 0 \ "$(grep -rh 'CALL UNSTUBBED' "$HERE/out-reactions" 2>/dev/null | wc -l | tr -d ' ')" diff --git a/test/seidroid-review/run-guard.sh b/test/seidroid-review/run-guard.sh new file mode 100755 index 0000000..12b0193 --- /dev/null +++ b/test/seidroid-review/run-guard.sh @@ -0,0 +1,625 @@ +#!/usr/bin/env bash +# Runs the guard's shipped step scripts under bash with a gh stub on PATH, and +# evaluates the shipped job conditions with gha.py. +# +# Every script and every condition is read out of the workflow on each run, so a +# run tests what the file says now and cannot pass against a stale copy. +set -uo pipefail +HERE="$(cd "$(dirname "$0")" && pwd)" +REPOROOT="$(cd "$HERE/../.." && pwd)" +WORKFLOW="$REPOROOT/.github/workflows/seidroid-review.yml" +ASSISTANT="$REPOROOT/.github/workflows/ai-assistant.yml" +REFUSE="$HERE/refuse.sh" +PARSE="$HERE/parse.sh" +ADMIT="$HERE/admit.sh" +# Named apart from the ack.sh and answer.sh reactions.sh writes. Both harnesses +# extract the same two steps, and a shared path lets one overwrite the other's +# extraction mid-run. +ACK="$HERE/guard-ack.sh" +ANSWER="$HERE/guard-answer.sh" +MARKER="$(python3 "$HERE/extract.py" "$WORKFLOW" "Refuse an event this workflow does not handle" "$REFUSE" VERDICT_MARKER)" || { + echo "could not read the refusal step out of $WORKFLOW"; exit 1; } +python3 "$HERE/extract.py" "$WORKFLOW" parse "$PARSE" VERDICT_MARKER >/dev/null || { + echo "could not read the parse step out of $WORKFLOW"; exit 1; } +python3 "$HERE/extract.py" "$WORKFLOW" "Admit the request" "$ADMIT" VERDICT_MARKER >/dev/null || { + echo "could not read the admission step out of $WORKFLOW"; exit 1; } +python3 "$HERE/extract.py" "$WORKFLOW" "Acknowledge the trigger" "$ACK" VERDICT_MARKER >/dev/null || { + echo "could not read the acknowledgement step out of $WORKFLOW"; exit 1; } +python3 "$HERE/extract.py" "$WORKFLOW" "Answer the request" "$ANSWER" VERDICT_MARKER >/dev/null || { + echo "could not read the answering step out of $WORKFLOW"; exit 1; } + +mkdir -p "$HERE/out" +CTX="$HERE/out/ctx.json" +PATH="$HERE/bin-guard:$PATH" +export PATH + +pass=0 fail=0 +check() { # label expected actual + if [ "$2" = "$3" ]; then pass=$((pass + 1)); else + fail=$((fail + 1)); echo " FAIL $1: want [$2] got [$3]"; fi +} + +# --- the job conditions ------------------------------------------------------- +# One context per case, built here rather than committed: a condition reads six +# payload fields and a context file per case would be six lines of JSON each. +ctx_comment() { # event action login type association allowed-bots + jq -nc --arg ev "$1" --arg action "$2" --arg login "$3" --arg type "$4" \ + --arg assoc "$5" --arg bots "$6" ' + {github: {event_name: $ev, + event: {action: $action, pull_request: {number: 7}}}, + inputs: {mode: "review", "allowed-bots": $bots}} + | .github.event |= (if $ev == "pull_request_review" + then . + {review: {id: 99, user: {login: $login, type: $type}, + author_association: $assoc, body: "@seidroid review"}} + else . + {comment: {id: 42, user: {login: $login, type: $type}, + author_association: $assoc, body: "@seidroid review"}} + end) + | if $ev == "issue_comment" + then .github.event.issue = {number: 7, pull_request: {url: "https://api/pulls/7"}} + else . end' > "$CTX" +} + +ctx_issue_only() { # an issue_comment on an ISSUE, which carries no pull_request + ctx_comment issue_comment created alice User MEMBER '[]' + jq -c '.github.event.issue.pull_request = null' "$CTX" > "$CTX.tmp" && mv "$CTX.tmp" "$CTX" +} + +ctx_plain() { # event mode [allowed-bots] + jq -nc --arg ev "$1" --arg mode "$2" --arg bots "${3-[]}" ' + {github: {event_name: $ev, event: {action: "opened", pull_request: {number: 7}}}, + inputs: {mode: $mode, "allowed-bots": $bots}}' > "$CTX" +} + +ctx_needs() { # event mode guard-result should_run + # The guard's four request outputs, with distinct values, so an assertion on a + # consumer names which output it read rather than only that it read something. + jq -nc --arg ev "$1" --arg mode "$2" --arg result "$3" --arg run "$4" ' + {github: {event_name: $ev, event: {pull_request: {number: 7}}}, + inputs: {mode: $mode}, + needs: {guard: {result: $result, + outputs: {should_run: $run, pr_number: "7", + comment_id: "111", comment_api: "pulls/comments", + trigger_id: "222"}}}}' > "$CTX" +} + +input_of_file() { # workflow input field + python3 "$HERE/gha.py" --input "$1" "$2" "$3" 2>/dev/null || printf 'error' +} + +input_of() { # input field + input_of_file "$WORKFLOW" "$1" "$2" +} + +env_of() { # step key -- against the context $CTX holds + python3 "$HERE/gha.py" --env "$WORKFLOW" "$1" "$2" "$CTX" 2>/dev/null || printf 'error' +} + +cond() { # job-id + local out + if out="$(python3 "$HERE/gha.py" "$WORKFLOW" "$1" "$CTX" 2>/dev/null)"; then + printf '%s' "$out" + else + printf 'error' + fi +} + +# --- the step scripts --------------------------------------------------------- +run_case() { # name script KEY=VALUE... + local name="$1" script="$2" + shift 2 + CASE="$HERE/out/$name" + rm -rf "$CASE" + mkdir -p "$CASE" + export STUB_LOG="$CASE/calls.log"; : > "$STUB_LOG" + export GITHUB_OUTPUT="$CASE/output.txt"; : > "$GITHUB_OUTPUT" + export VERDICT_MARKER="$MARKER" + # What `parse` reads. + export EVENT_NAME=issue_comment + export BODY='@seidroid review' + export PR_NUMBER=7 + export COMMENT_ID=42 + export TRIGGER_PHRASE='@seidroid' + # What `Admit the request` reads. + export GH_TOKEN=app-token GATE_TOKEN=app-token + export ALLOWED_TEAM=sei-protocol/sei-core + export ALLOWED_BOTS='[]' + export SKIP_LABEL='ai: skip-review' + export ACTOR=alice ACTOR_TYPE=User + export REPO=owner/repo PR=7 PARSED=true + export IS_DRAFT=false ACTION=created RE_REVIEW_ON_PUSH=false MODE=review + export HEAD_REPO_ID='' BASE_REPO_ID='' + # What the two reacting steps read. + export TRIGGER_REPO=owner/repo TRIGGER_ID=42 COMMENT_API=issues/comments + export CHECK="$CASE/check.json" VERDICT_PRODUCED=true + printf '%s\n' '{"conclusion":"success"}' > "$CASE/check.json" + # What the gh stub answers. + export STUB_TEAM=active STUB_ORIGIN=same STUB_LABELS='' STUB_REVIEWS=none STUB_COMMENTS=none + export STUB_REACTIONS=none + for kv in "$@"; do export "${kv?}"; done + bash "$script" > "$CASE/stdout.txt" 2> "$CASE/stderr.txt" + echo "$?" > "$CASE/rc" +} + +rc() { cat "$CASE/rc"; } +out() { grep -E "^$1=" "$CASE/output.txt" | tail -1 | cut -d= -f2-; } +written() { grep -cE "^$1=" "$CASE/output.txt"; } +calls() { grep -c "^CALL $1" "$CASE/calls.log" || true; } +said() { grep -c -- "$1" "$CASE/stdout.txt" || true; } +cried() { grep -c -- "$1" "$CASE/stderr.txt" || true; } + +echo "== 0. the expression model gha.py holds ==" +python3 "$HERE/gha.py" --selftest +check "expression model clean" 0 "$?" + +echo +echo "== 1. the guard's condition: which requests reach a runner ==" +ctx_comment issue_comment created alice User MEMBER '[]' +check "issue_comment, member" true "$(cond guard)" +ctx_comment pull_request_review_comment created alice User MEMBER '[]' +check "diff-thread comment, member" true "$(cond guard)" +ctx_comment pull_request_review submitted alice User MEMBER '[]' +check "review body, member" true "$(cond guard)" +ctx_comment pull_request_review_comment edited alice User MEMBER '[]' +check "an edited diff-thread comment" false "$(cond guard)" +ctx_comment pull_request_review dismissed alice User MEMBER '[]' +check "a dismissed review" false "$(cond guard)" +ctx_comment pull_request_review edited alice User MEMBER '[]' +check "an edited review" false "$(cond guard)" +ctx_comment pull_request_review submitted mallory User NONE '[]' +check "review body, outsider" false "$(cond guard)" +ctx_comment pull_request_review_comment created mallory User NONE '[]' +check "diff-thread comment, outsider" false "$(cond guard)" +ctx_comment pull_request_review submitted alice User COLLABORATOR '[]' +check "review body, collaborator" true "$(cond guard)" +ctx_comment pull_request_review_comment created alice User OWNER '[]' +check "diff-thread comment, owner" true "$(cond guard)" +ctx_issue_only +check "a comment on an issue" false "$(cond guard)" + +echo +echo "== 2. the guard's condition: allowed-bots ==" +ctx_comment issue_comment created 'dependabot[bot]' Bot MEMBER '[]' +check "empty list denies a bot" false "$(cond guard)" +ctx_comment issue_comment created 'dependabot[bot]' Bot NONE '["dependabot[bot]"]' +check "a listed bot" true "$(cond guard)" +ctx_comment issue_comment created 'dependabot[bot]' Bot NONE '["renovate[bot]"]' +check "an unlisted bot" false "$(cond guard)" +ctx_comment issue_comment created 'DependaBot[BOT]' Bot NONE '["dependabot[bot]"]' +check "one login, either case" true "$(cond guard)" +# `bot` is a substring of every listed login here. A condition that tested the +# input as a STRING would admit it; membership of the parsed array does not. +ctx_comment issue_comment created 'bot' Bot NONE '["dependabot[bot]","renovate[bot]"]' +check "a login inside a listed one" false "$(cond guard)" +ctx_comment pull_request_review submitted 'dependabot[bot]' Bot NONE '["dependabot[bot]"]' +check "a listed bot in a review body" true "$(cond guard)" +ctx_comment pull_request_review_comment created 'dependabot[bot]' Bot NONE '["dependabot[bot]"]' +check "a listed bot in a diff thread" true "$(cond guard)" +# A value fromJSON cannot read, on the two requesters that reach it differently. +# The runner short-circuits, so a person is admitted on association before the list +# is parsed at all; a bot is the requester whose admission depends on parsing it. A +# caller wiring error therefore fails the requests it governs and no others. +ctx_comment issue_comment created alice User MEMBER 'not json' +check "a person never reads the list" true "$(cond guard)" +ctx_comment issue_comment created 'dependabot[bot]' Bot NONE 'not json' +check "a bot reads it, and it fails loudly" error "$(cond guard)" +ctx_plain pull_request review 'not json' +check "an automatic review never reads it" true "$(cond guard)" +# An UNSET input, which is the likelier accident: a workflow_call default applies +# only to an input the caller omits, so `allowed-bots: ${{ vars.UNSET }}` arrives as +# the empty string and fromJSON('') is not []. Empty has to read as the documented +# default -- deny every bot -- rather than take the run down. +ctx_comment issue_comment created 'dependabot[bot]' Bot NONE '' +check "an unset list denies every bot" false "$(cond guard)" +ctx_comment issue_comment created 'dependabot[bot]' Bot MEMBER '' +check "and association is no way round it" false "$(cond guard)" +ctx_comment issue_comment created alice User MEMBER '' +check "and a person is still admitted" true "$(cond guard)" +ctx_plain pull_request review '' +check "and an automatic review still runs" true "$(cond guard)" + +echo +echo "== 3. the guard's condition: the events it does not handle ==" +ctx_plain pull_request_target review +check "pull_request_target reaches the guard" true "$(cond guard)" +ctx_plain push review +check "push reaches the guard" true "$(cond guard)" +ctx_plain workflow_dispatch review +check "workflow_dispatch reaches the guard" true "$(cond guard)" +ctx_plain pull_request review +check "an automatic review" true "$(cond guard)" +# A pull_request close skips this guard, and the review job reads that skip as +# its own trigger. Admitting it here would break the only path that reclaims a +# sandbox. +ctx_plain pull_request close +check "a pull_request close skips the guard" false "$(cond guard)" + +echo +echo "== 4. the review job's condition ==" +ctx_needs issue_comment review success true +check "issue_comment, admitted" true "$(cond review)" +ctx_needs pull_request_review_comment review success true +check "diff-thread comment, admitted" true "$(cond review)" +ctx_needs pull_request_review review success true +check "review body, admitted" true "$(cond review)" +ctx_needs pull_request_review review success false +check "review body, refused" false "$(cond review)" +ctx_needs pull_request_review_comment review failure '' +check "diff-thread comment, guard failed" false "$(cond review)" +ctx_needs pull_request_target review failure '' +check "pull_request_target never reviews" false "$(cond review)" +ctx_needs pull_request review success true +check "an automatic review, admitted" true "$(cond review)" +ctx_needs pull_request close skipped '' +check "a pull_request close still runs" true "$(cond review)" + +echo +echo "== 5. the refusal: an event this workflow does not handle ==" +run_case refuse-target "$REFUSE" EVENT_NAME=pull_request_target +check "rc" 1 "$(rc)" +check "names the event" 1 "$(cried pull_request_target)" +check "says why" 1 "$(cried "base repository's secrets")" +check "names the four it handles" 1 "$(cried "pull_request, issue_comment, pull_request_review_comment or pull_request_review")" +run_case refuse-push "$REFUSE" EVENT_NAME=push +check "rc" 1 "$(rc)" +check "names the event" 1 "$(cried "cannot be called from 'push'")" +check "names the four it handles" 1 "$(cried "pull_request, issue_comment, pull_request_review_comment and pull_request_review")" +run_case refuse-dispatch "$REFUSE" EVENT_NAME=workflow_dispatch +check "rc" 1 "$(rc)" +check "names the event" 1 "$(cried workflow_dispatch)" +for ev in pull_request issue_comment pull_request_review_comment pull_request_review; do + run_case "refuse-ok-$ev" "$REFUSE" "EVENT_NAME=$ev" + check "$ev passes" 0 "$(rc)" + check "$ev says nothing" 0 "$(cried '::')" +done + +echo +echo "== 6. the parse: which body is a command, and what it resolves to ==" +run_case parse-issue "$PARSE" EVENT_NAME=issue_comment +check "should_run" true "$(out should_run)" +check "comment_id" 42 "$(out comment_id)" +check "comment_api" issues/comments "$(out comment_api)" +check "trigger_id" 42 "$(out trigger_id)" +run_case parse-thread "$PARSE" EVENT_NAME=pull_request_review_comment +check "should_run" true "$(out should_run)" +check "comment_id" 42 "$(out comment_id)" +check "comment_api" pulls/comments "$(out comment_api)" +check "trigger_id" 42 "$(out trigger_id)" +run_case parse-review "$PARSE" EVENT_NAME=pull_request_review COMMENT_ID=99 +check "should_run" true "$(out should_run)" +check "comment_id held back" '' "$(out comment_id)" +check "comment_id written once" 1 "$(written comment_id)" +check "comment_api held back" '' "$(out comment_api)" +# The log id is a different fact from the reactable id, and this is the event where +# they differ: nothing can react on a review, but the run still has a request to name. +check "trigger_id still carries the id" 99 "$(out trigger_id)" +check "says the request earns no reaction" 1 "$(said 'carries no reactions endpoint')" +run_case parse-auto "$PARSE" EVENT_NAME=pull_request BODY='' +check "should_run" true "$(out should_run)" +check "comment_id" '' "$(out comment_id)" +check "comment_api written" 1 "$(written comment_api)" +check "trigger_id" '' "$(out trigger_id)" +run_case parse-bare "$PARSE" BODY='seidroid review' +check "the bare phrase still asks" true "$(out should_run)" +run_case parse-prose "$PARSE" BODY='Do we need @seidroid review on this one?' +check "prose about the command" false "$(out should_run)" +run_case parse-close "$PARSE" BODY='@seidroid review close' +check "a close" true "$(out should_run)" +run_case parse-multiline "$PARSE" BODY='Looks good otherwise. +@seidroid review +Thanks!' +check "a command on its own line" true "$(out should_run)" +run_case parse-target "$PARSE" BODY='@seidroid review owner/repo#5' +check "a named repository" false "$(out should_run)" +check "and it says so" 1 "$(said 'takes no repository target')" +run_case parse-padded "$PARSE" BODY=' @seidroid review ' +check "padding around the command" true "$(out should_run)" +run_case parse-suffix "$PARSE" BODY='@seidroidX review' +check "a longer login" false "$(out should_run)" +run_case parse-target-own "$PARSE" TRIGGER_PHRASE='@mybot' BODY='@mybot review owner/repo#5' +check "a named repository, own phrase" false "$(out should_run)" +check "and it says so" 1 "$(said 'takes no repository target')" + +echo +echo "== 7. the parse: a caller's own trigger phrase ==" +run_case phrase-own "$PARSE" TRIGGER_PHRASE='@mybot' BODY='@mybot review' +check "the phrase the caller set" true "$(out should_run)" +check "no warning" 0 "$(said '::warning')" +run_case phrase-not-default "$PARSE" TRIGGER_PHRASE='@mybot' BODY='@seidroid review' +check "and not the default" false "$(out should_run)" +run_case phrase-hyphen "$PARSE" TRIGGER_PHRASE='@sei-droid' BODY='@sei-droid review' +check "a hyphen is part of the phrase" true "$(out should_run)" +check "no warning" 0 "$(said '::warning')" +run_case phrase-bare-ok "$PARSE" TRIGGER_PHRASE='mybot' BODY='mybot review' +check "a phrase written without the @" true "$(out should_run)" +run_case phrase-case "$PARSE" TRIGGER_PHRASE='@Seidroid' BODY='@Seidroid review' +check "the phrase is matched as written" true "$(out should_run)" +run_case phrase-case-other "$PARSE" TRIGGER_PHRASE='@Seidroid' BODY='@seidroid review' +check "and not in another case" false "$(out should_run)" +# A `.` reaching the pattern unconstrained matches any character, so `@myXbot` +# would ask for a review under a phrase nobody configured. +run_case phrase-dot "$PARSE" TRIGGER_PHRASE='@my.bot' BODY='@myXbot review' +check "a dot cannot stand for a character" false "$(out should_run)" +check "and the phrase is refused" 1 "$(said "trigger-phrase '@my.bot' is not")" +run_case phrase-dot-fallback "$PARSE" TRIGGER_PHRASE='@my.bot' BODY='@seidroid review' +check "the default takes over" true "$(out should_run)" +# A `|` reaching the pattern unconstrained splits it into two alternatives, and +# the first is `^[[:space:]]*@?a` -- which any line starting with `a` matches. +run_case phrase-pipe "$PARSE" TRIGGER_PHRASE='@a|b' BODY='a note about the diff' +check "a pipe cannot widen the match" false "$(out should_run)" +check "and the phrase is refused" 1 "$(said "trigger-phrase '@a|b' is not")" +run_case phrase-pipe-fallback "$PARSE" TRIGGER_PHRASE='@a|b' BODY='@seidroid review' +check "the default takes over" true "$(out should_run)" +run_case phrase-empty "$PARSE" TRIGGER_PHRASE='' BODY='@seidroid review' +check "an empty phrase falls back" true "$(out should_run)" +check "and says so" 1 "$(said "trigger-phrase '' is not")" +run_case phrase-space "$PARSE" TRIGGER_PHRASE='@my bot' BODY='@seidroid review' +check "a phrase with a space falls back" true "$(out should_run)" +run_case phrase-at-only "$PARSE" TRIGGER_PHRASE='@' BODY='@seidroid review' +check "a bare @ falls back" true "$(out should_run)" + +echo +echo "== 8. the admission: who may ask, on every comment path ==" +run_case admit-member "$ADMIT" EVENT_NAME=issue_comment +check "admit" true "$(out admit)" +check "the team was read" 1 "$(calls membership)" +run_case admit-thread "$ADMIT" EVENT_NAME=pull_request_review_comment +check "admit" true "$(out admit)" +check "the team was read" 1 "$(calls membership)" +check "the origin was read" 1 "$(calls origin)" +check "the labels were read" 1 "$(calls labels)" +run_case admit-reviewbody "$ADMIT" EVENT_NAME=pull_request_review +check "admit" true "$(out admit)" +check "the team was read" 1 "$(calls membership)" +check "the origin was read" 1 "$(calls origin)" +check "the labels were read" 1 "$(calls labels)" +run_case admit-pending "$ADMIT" EVENT_NAME=issue_comment STUB_TEAM=pending +check "admit" false "$(out admit)" +check "and names the team" 1 "$(said 'not an active member of sei-protocol/sei-core')" +run_case admit-thread-pending "$ADMIT" EVENT_NAME=pull_request_review_comment STUB_TEAM=pending +check "a diff thread is no way round it" false "$(out admit)" +run_case admit-review-pending "$ADMIT" EVENT_NAME=pull_request_review STUB_TEAM=pending +check "a review body is no way round it" false "$(out admit)" +run_case admit-team-fail "$ADMIT" EVENT_NAME=pull_request_review STUB_TEAM=FAIL +check "a membership read that fails" false "$(out admit)" +run_case admit-no-team "$ADMIT" EVENT_NAME=pull_request_review ALLOWED_TEAM='' +check "an empty team denies" false "$(out admit)" +check "and reads nothing" 0 "$(calls membership)" +run_case admit-no-app "$ADMIT" EVENT_NAME=pull_request_review_comment GH_TOKEN='' +check "no App identity denies" false "$(out admit)" +check "and names the two secrets" 1 "$(said 'SEIDROID_APP_ID and SEIDROID_APP_PRIVATE_KEY')" +check "and reads nothing" 0 "$(calls membership)" + +echo +echo "== 9. the admission: a bot is held to allowed-bots ==" +run_case bot-listed "$ADMIT" ACTOR='dependabot[bot]' ACTOR_TYPE=Bot ALLOWED_BOTS='["dependabot[bot]"]' +check "admit" true "$(out admit)" +check "the team is not read for a bot" 0 "$(calls membership)" +run_case bot-case "$ADMIT" ACTOR='DependaBot[BOT]' ACTOR_TYPE=Bot ALLOWED_BOTS='["dependabot[bot]"]' +check "one login, either case" true "$(out admit)" +run_case bot-type-case "$ADMIT" ACTOR='dependabot[bot]' ACTOR_TYPE=bot ALLOWED_BOTS='["dependabot[bot]"]' +check "the type is read either case" true "$(out admit)" +check "the team is not read" 0 "$(calls membership)" +run_case bot-unlisted "$ADMIT" ACTOR='renovate[bot]' ACTOR_TYPE=Bot ALLOWED_BOTS='["dependabot[bot]"]' +check "an unlisted bot" false "$(out admit)" +check "and names the input" 1 "$(said 'is not in allowed-bots')" +check "and reads no team" 0 "$(calls membership)" +run_case bot-empty-list "$ADMIT" ACTOR='dependabot[bot]' ACTOR_TYPE=Bot ALLOWED_BOTS='[]' +check "the default list denies" false "$(out admit)" +run_case bot-substring "$ADMIT" ACTOR='bot' ACTOR_TYPE=Bot ALLOWED_BOTS='["dependabot[bot]"]' +check "a login inside a listed one" false "$(out admit)" +run_case bot-prefix "$ADMIT" ACTOR='dependabot[bot]x' ACTOR_TYPE=Bot ALLOWED_BOTS='["dependabot[bot]"]' +check "a login that extends a listed one" false "$(out admit)" +run_case bot-not-json "$ADMIT" ACTOR='dependabot[bot]' ACTOR_TYPE=Bot ALLOWED_BOTS='not json' +check "a list that is not JSON" false "$(out admit)" +run_case bot-not-array "$ADMIT" ACTOR='dependabot[bot]' ACTOR_TYPE=Bot ALLOWED_BOTS='"dependabot[bot]"' +check "a string where an array belongs" false "$(out admit)" +run_case bot-numbers "$ADMIT" ACTOR='123' ACTOR_TYPE=Bot ALLOWED_BOTS='[123]' +check "a list of numbers matches nothing" false "$(out admit)" +run_case bot-no-actor "$ADMIT" ACTOR='' ACTOR_TYPE=Bot ALLOWED_BOTS='["dependabot[bot]"]' +check "an empty login matches nothing" false "$(out admit)" +run_case bot-null-list "$ADMIT" ACTOR='dependabot[bot]' ACTOR_TYPE=Bot ALLOWED_BOTS='[null]' +check "a list of nulls matches nothing" false "$(out admit)" +run_case bot-review "$ADMIT" EVENT_NAME=pull_request_review ACTOR='dependabot[bot]' ACTOR_TYPE=Bot \ + ALLOWED_BOTS='["dependabot[bot]"]' +check "a listed bot in a review body" true "$(out admit)" +run_case bot-thread "$ADMIT" EVENT_NAME=pull_request_review_comment ACTOR='dependabot[bot]' ACTOR_TYPE=Bot \ + ALLOWED_BOTS='["dependabot[bot]"]' +check "a listed bot in a diff thread" true "$(out admit)" +run_case bot-fork "$ADMIT" ACTOR='dependabot[bot]' ACTOR_TYPE=Bot ALLOWED_BOTS='["dependabot[bot]"]' \ + STUB_ORIGIN=fork +check "a listed bot still meets the fork check" false "$(out admit)" +run_case bot-label "$ADMIT" ACTOR='dependabot[bot]' ACTOR_TYPE=Bot ALLOWED_BOTS='["dependabot[bot]"]' \ + STUB_LABELS='ai: skip-review' +check "a listed bot still meets the label" false "$(out admit)" + +echo +echo "== 10. the admission: the rules that were already there, on the new paths ==" +run_case fork-thread "$ADMIT" EVENT_NAME=pull_request_review_comment STUB_ORIGIN=fork +check "a fork in a diff thread" false "$(out admit)" +check "and names the refusal" 1 "$(said 'explicit re-reviews are disabled for fork-originated')" +run_case fork-review "$ADMIT" EVENT_NAME=pull_request_review STUB_ORIGIN=fork +check "a fork in a review body" false "$(out admit)" +run_case fork-unreadable "$ADMIT" EVENT_NAME=pull_request_review STUB_ORIGIN=FAIL +check "an origin nobody could read" false "$(out admit)" +check "and says a fork is not ruled out" 1 "$(said 'could not read where owner/repo#7 comes from')" +run_case fork-null-head "$ADMIT" EVENT_NAME=pull_request_review_comment STUB_ORIGIN=null +check "a null head repository reads as a fork" false "$(out admit)" +run_case label-thread "$ADMIT" EVENT_NAME=pull_request_review_comment STUB_LABELS='ai: skip-review' +check "the skip label in a diff thread" false "$(out admit)" +check "and names the label" 1 "$(said 'carries ai: skip-review')" +run_case label-review "$ADMIT" EVENT_NAME=pull_request_review STUB_LABELS='ai: skip-review' +check "the skip label in a review body" false "$(out admit)" +run_case label-other "$ADMIT" EVENT_NAME=pull_request_review STUB_LABELS='needs-rebase,ai: nitpick' +check "another label admits" true "$(out admit)" +run_case label-fail "$ADMIT" EVENT_NAME=pull_request_review STUB_LABELS=FAIL +check "a label read that fails" false "$(out admit)" +check "and names both fixes" 1 "$(said 'Grant pull-requests: read on the calling job')" +run_case gate-review "$ADMIT" EVENT_NAME=pull_request_review STUB_COMMENTS=verdict +check "a review already ran, asked by name" true "$(out admit)" +check "and the gate reads nothing" 0 "$(calls comments)" +run_case gate-thread "$ADMIT" EVENT_NAME=pull_request_review_comment STUB_COMMENTS=verdict +check "the same from a diff thread" true "$(out admit)" +run_case not-parsed "$ADMIT" EVENT_NAME=pull_request_review PARSED=false +check "a body that is no command" false "$(out admit)" +check "and reads nothing at all" 0 "$(( $(calls membership) + $(calls origin) + $(calls labels) ))" + +echo +echo "== 11. the admission: the paths that were already there ==" +run_case close-bot "$ADMIT" MODE=close ACTOR='renovate[bot]' ACTOR_TYPE=Bot ALLOWED_BOTS='[]' +check "a teardown is not refused" true "$(out admit)" +check "and checks nothing" 0 "$(( $(calls membership) + $(calls origin) + $(calls labels) ))" +run_case close-outsider "$ADMIT" MODE=close STUB_TEAM=pending STUB_ORIGIN=fork STUB_LABELS='ai: skip-review' +check "a teardown from outside the team" true "$(out admit)" +run_case auto-draft "$ADMIT" EVENT_NAME=pull_request IS_DRAFT=true ACTION=opened BASE_REPO_ID=1 HEAD_REPO_ID=1 +check "a draft" false "$(out admit)" +check "and names it" 1 "$(said 'is a draft; not reviewing')" +run_case auto-first "$ADMIT" EVENT_NAME=pull_request ACTION=opened BASE_REPO_ID=1 HEAD_REPO_ID=1 +check "a first automatic review" true "$(out admit)" +check "and reads no team" 0 "$(calls membership)" +run_case auto-again "$ADMIT" EVENT_NAME=pull_request ACTION=synchronize BASE_REPO_ID=1 HEAD_REPO_ID=1 \ + STUB_COMMENTS=verdict +check "a push after a verdict" false "$(out admit)" +check "and points at the comment" 1 "$(said 'comment @seidroid review to ask for one')" +run_case auto-fork "$ADMIT" EVENT_NAME=pull_request ACTION=opened BASE_REPO_ID=1 HEAD_REPO_ID=2 +check "a fork pull request" false "$(out admit)" +check "and spends no API call" 0 "$(calls origin)" + +echo +echo "== 12. the payload field each step reads, per event ==" +ctx_comment issue_comment created alice User MEMBER '[]' +check "parse reads the comment body" '@seidroid review' "$(env_of parse BODY)" +check "parse reads the comment id" 42 "$(env_of parse COMMENT_ID)" +check "parse reads the pull request number" 7 "$(env_of parse PR_NUMBER)" +check "admit reads the commenter" alice "$(env_of "Admit the request" ACTOR)" +check "admit reads the commenter's type" User "$(env_of "Admit the request" ACTOR_TYPE)" +ctx_comment pull_request_review_comment created alice User MEMBER '[]' +check "parse reads the thread comment body" '@seidroid review' "$(env_of parse BODY)" +check "parse reads the thread comment id" 42 "$(env_of parse COMMENT_ID)" +check "parse reads the pull request number" 7 "$(env_of parse PR_NUMBER)" +check "admit reads the commenter" alice "$(env_of "Admit the request" ACTOR)" +ctx_comment pull_request_review submitted alice User MEMBER '[]' +# The review event names none of these under `comment`, so a step reading that +# key alone would see an empty body, parse no command, and refuse in silence. +check "parse reads the review body" '@seidroid review' "$(env_of parse BODY)" +check "parse reads the review id" 99 "$(env_of parse COMMENT_ID)" +check "parse reads the pull request number" 7 "$(env_of parse PR_NUMBER)" +check "admit reads the reviewer" alice "$(env_of "Admit the request" ACTOR)" +check "admit reads the reviewer's type" User "$(env_of "Admit the request" ACTOR_TYPE)" +ctx_comment pull_request_review submitted 'dependabot[bot]' Bot NONE '["dependabot[bot]"]' +check "admit reads a review bot's login" 'dependabot[bot]' "$(env_of "Admit the request" ACTOR)" +check "admit reads a review bot's type" Bot "$(env_of "Admit the request" ACTOR_TYPE)" + +echo +echo "== 12b. each consumer reads the output meant for it ==" +ctx_needs issue_comment review success true +check "the driver labels with trigger_id" 222 \ + "$(env_of "Drive session + collect verdict" TRIGGER_ID)" +check "the acknowledgement reacts on comment_id" 111 \ + "$(env_of "Acknowledge the trigger" TRIGGER_ID)" +check "and on the collection beside it" pulls/comments \ + "$(env_of "Acknowledge the trigger" COMMENT_API)" +check "the answer reads the same two" "111 pulls/comments" \ + "$(env_of "Answer the request" TRIGGER_ID) $(env_of "Answer the request" COMMENT_API)" +check "so does the withdrawal" "111 pulls/comments" \ + "$(env_of "Withdraw the reactions on a cancelled run" TRIGGER_ID) $(env_of "Withdraw the reactions on a cancelled run" COMMENT_API)" + +echo +echo "== 13. the defaults a caller inherits ==" +check "trigger-phrase default" '@seidroid' "$(input_of trigger-phrase default)" +check "trigger-phrase optional" false "$(input_of trigger-phrase required)" +check "allowed-bots default" '[]' "$(input_of allowed-bots default)" +check "allowed-bots optional" false "$(input_of allowed-bots required)" +check "allowed-team default" 'sei-protocol/sei-core' "$(input_of allowed-team default)" + +echo +echo "== 14. the acknowledgement lands on the object that asked ==" +run_case ack-issue "$ACK" COMMENT_API=issues/comments TRIGGER_ID=42 +check "the issue comments collection" \ + "CALL reaction POST repos/owner/repo/issues/comments/42/reactions" "$(cat "$CASE/calls.log")" +check "and reports it" 1 "$(said 'acknowledged comment 42')" +run_case ack-thread "$ACK" COMMENT_API=pulls/comments TRIGGER_ID=77 +check "the pull comments collection" \ + "CALL reaction POST repos/owner/repo/pulls/comments/77/reactions" "$(cat "$CASE/calls.log")" +run_case ack-fails "$ACK" COMMENT_API=pulls/comments STUB_REACTIONS=FAIL +check "a reaction that does not post warns" 1 "$(said '::warning::could not react to comment 42')" +check "and the review goes on" 0 "$(rc)" + +echo +echo "== 15. the answer withdraws from the same collection ==" +run_case answer-issue "$ANSWER" COMMENT_API=issues/comments STUB_REACTIONS=eyes +check "read from the issue collection" 1 \ + "$(grep -c '^CALL reaction GET repos/owner/repo/issues/comments/42/reactions' "$CASE/calls.log")" +check "withdrew the eyes there" 1 \ + "$(grep -c '^CALL reaction DELETE repos/owner/repo/issues/comments/42/reactions/1' "$CASE/calls.log")" +check "and thumbed up there" 1 \ + "$(grep -c '^CALL reaction POST repos/owner/repo/issues/comments/42/reactions' "$CASE/calls.log")" +run_case answer-thread "$ANSWER" COMMENT_API=pulls/comments STUB_REACTIONS=eyes +check "read from the pull collection" 1 \ + "$(grep -c '^CALL reaction GET repos/owner/repo/pulls/comments/42/reactions' "$CASE/calls.log")" +check "withdrew the eyes there" 1 \ + "$(grep -c '^CALL reaction DELETE repos/owner/repo/pulls/comments/42/reactions/1' "$CASE/calls.log")" +check "and thumbed up there" 1 \ + "$(grep -c '^CALL reaction POST repos/owner/repo/pulls/comments/42/reactions' "$CASE/calls.log")" +check "nothing reached the other collection" 0 "$(grep -c 'issues/comments' "$CASE/calls.log")" + +echo +echo "== 16. what ai-assistant.yml claims of the same body, on every event ==" +# The two tools read one comment, on the same three events. This group evaluates the +# assistant's own reply condition beside the parse above, so which of them answers a +# given body is measured rather than reasoned about. Both conditions read one phrase, +# and the reasoning holds only while the two defaults agree. +check "the assistant takes the same phrase" '@seidroid' "$(input_of_file "$ASSISTANT" trigger-phrase default)" +claims() { # event body -- true when the assistant's reply job would run + # Per event, because the assistant has a branch each and this branch keys on the + # payload key the event populates. A helper that named one event would measure the + # overlap on the path that already had it, and infer the two this workflow adds. + jq -nc --arg ev "$1" --arg b "$2" ' + {github: {event_name: $ev, event: {}}, + inputs: {"trigger-phrase": "@seidroid"}} + | .github.event |= (if $ev == "pull_request_review" + then {review: {body: $b, user: {type: "User"}}} + else {comment: {body: $b, user: {type: "User"}}} end) + | if $ev == "issue_comment" + then .github.event.issue = {number: 7, pull_request: {url: "u"}} + else . end' > "$CTX" + python3 "$HERE/gha.py" "$ASSISTANT" reply "$CTX" 2>/dev/null || printf 'error' +} +# Every body is checked on all three events, because this workflow now answers all +# three and the division of labour has to hold on each. +for ev in issue_comment pull_request_review_comment pull_request_review; do + # The bare form is the widening this workflow keeps. The assistant needs the @ to + # claim a body at all, so nothing else answers it, on any event. + run_case "claim-bare-$ev" "$PARSE" "EVENT_NAME=$ev" BODY='seidroid review' + check "$ev bare: this workflow" true "$(out should_run)" + check "$ev bare: the assistant" false "$(claims "$ev" 'seidroid review')" + run_case "claim-exact-$ev" "$PARSE" "EVENT_NAME=$ev" BODY='@seidroid review' + check "$ev exact: this workflow" true "$(out should_run)" + check "$ev exact: the assistant" false "$(claims "$ev" '@seidroid review')" + run_case "claim-prose-$ev" "$PARSE" "EVENT_NAME=$ev" BODY='Do we need @seidroid review here?' + check "$ev prose: this workflow" false "$(out should_run)" + check "$ev prose: the assistant" true "$(claims "$ev" 'Do we need @seidroid review here?')" + # Two bodies both tools answer. Whole-line anchoring is what admits the second, and + # it is also what keeps the prose above from starting a review, so the overlap is + # the price of that. The assistant reserves the exact body, and neither of these is + # it. This branch widens both onto the two events it adds, which is why each is + # asserted per event rather than once. + run_case "claim-close-$ev" "$PARSE" "EVENT_NAME=$ev" BODY='@seidroid review close' + check "$ev close: this workflow" true "$(out should_run)" + check "$ev close: the assistant too" true "$(claims "$ev" '@seidroid review close')" + run_case "claim-multiline-$ev" "$PARSE" "EVENT_NAME=$ev" BODY='Looks good. +@seidroid review +Thanks!' + check "$ev amid prose: this workflow" true "$(out should_run)" + check "$ev amid prose: the assistant too" true "$(claims "$ev" 'Looks good. +@seidroid review +Thanks!')" +done +# A review with no body at all. The assistant names that case; this workflow reads an +# empty body as no command. +run_case claim-empty-review "$PARSE" EVENT_NAME=pull_request_review BODY='' +check "an empty review body: this workflow" false "$(out should_run)" +check "an empty review body: the assistant" false "$(claims pull_request_review '')" + +echo +echo "assertions: $pass passed, $fail failed" +[ "$fail" -eq 0 ]