From c00cac05d5312877dc58baa0eea7fa5daf6c98fb Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 7 Sep 2026 12:31:36 -0700 Subject: [PATCH 1/7] fix(seidroid-review): clear the reactions on a cancelled run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two `@seidroid review` comments in quick succession cancel the first run. The answering step took `!cancelled()`, so the concurrency group skipped it, and the acknowledgement 👀 was already on the older comment. The newer run answers its own comment id, so nothing ever read the older one again and it wore the eyes for good. `always()` on that step, which is safe because the arm it admits is the one that already handles an absent conclusion: a cancelled run reaches the step with no check file, and that arm withdraws all three of this bot's reactions and posts no thumb. It spends four API calls, takes no position on the pull request, and starts none of the work a cancellation stops. `issues: write` stands. PLT-1159 asked to drop it and react through GraphQL `addReaction` under `pull-requests: write`. The incumbent evidence does not support that: ai-review's reactions are posted by the App installation token, not by GITHUB_TOKEN, so its `permissions:` block never governed them. GitHub publishes no permission data for any GraphQL mutation. The permission comment now records both facts. PLT-1166 Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/seidroid-review.yml | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/.github/workflows/seidroid-review.yml b/.github/workflows/seidroid-review.yml index f93d3d2..0eb72bf 100644 --- a/.github/workflows/seidroid-review.yml +++ b/.github/workflows/seidroid-review.yml @@ -974,7 +974,11 @@ jobs: 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. + # 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 + # permission for any mutation, so only a live call settles what that route needs. issues: write # acknowledge the trigger with a reaction # The credential lives ONLY here, at job level. It must never be re-declared # as step-level env on a `uses:` step (composite/action steps do not receive @@ -2395,13 +2399,26 @@ jobs: # owed an answer even then. Everything here needs only GITHUB_TOKEN and the # job's issues: write. # - # The condition names three facts: a review turn, not cancelled, and a comment - # to answer. A close produces no verdict, so a request to tear a session down - # earns no answer. + # The condition names two facts: a review turn, and a comment to answer. A + # close produces no verdict, so a request to tear a session down earns no + # answer. + # + # always(), where every other step in this job takes !cancelled(). A newer + # `@seidroid review` cancels the run in flight, and the eyes are on the older + # comment by then. The newer run answers its OWN comment id, so a step that + # skips on cancellation leaves that older request wearing eyes nothing clears. + # + # What always() admits is a run with no check file, and the missing-conclusion + # arm below already answers it: withdraw all three of this bot's reactions, + # post nothing. Cheap to admit. That arm spends four API calls and takes no + # position on the pull request. It starts none of the work a cancellation is + # there to stop. A runner shutdown skips every remaining condition, so nothing + # in this job survives that. + # # No verdict_produced gate: the acknowledgement waits on this step, and a re-run # replays the trigger comment id, so a run reaching no verdict still has to clear # the eyes, and a thumb an earlier attempt left there. - if: ${{ inputs.mode == 'review' && !cancelled() + if: ${{ inputs.mode == 'review' && always() && needs.guard.outputs.comment_id != '' }} continue-on-error: true shell: bash From dd6a108a6c2d8ebae335d8a7ad3273ab7c55919a Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 7 Sep 2026 12:36:17 -0700 Subject: [PATCH 2/7] docs(seidroid-review): say what the other steps do on a cancelled run Three steps in the review job carry no `if:` at all, so the runner wraps them as `success()`. Naming `!cancelled()` as what every other step takes was wrong. All fifteen skip a cancelled run, whichever route they take there, and that is the fact the reader needs. PLT-1166 Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/seidroid-review.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/seidroid-review.yml b/.github/workflows/seidroid-review.yml index 0eb72bf..f771477 100644 --- a/.github/workflows/seidroid-review.yml +++ b/.github/workflows/seidroid-review.yml @@ -2403,7 +2403,7 @@ jobs: # close produces no verdict, so a request to tear a session down earns no # answer. # - # always(), where every other step in this job takes !cancelled(). A newer + # always(), where every other step in this job skips a cancelled run. A newer # `@seidroid review` cancels the run in flight, and the eyes are on the older # comment by then. The newer run answers its OWN comment id, so a step that # skips on cancellation leaves that older request wearing eyes nothing clears. From ec0657a42f7950dbf3e1d04f9ad9345283fe1b74 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 7 Sep 2026 12:56:20 -0700 Subject: [PATCH 3/7] fix(seidroid-review): withdraw the reactions in a step a cancelled run cannot post from A cancellation arriving after `drive` finishes leaves check_path and verdict_produced populated. `always()` on `Answer the request` therefore read a real conclusion and thumbed the request, while `Post the verdict` skipped and the verdict never reached the pull request. A thumb reads as an answer, so that is worse than the stale eyes it was meant to fix. `Answer the request` keeps `!cancelled()`. The withdrawal moves to a step of its own, last in the job, under `cancelled()`. That step reads no check file, no verdict_produced and no conclusion, so no value a cancellation leaves populated can make it state an outcome. Last in the job rather than beside the step above, because a condition is evaluated when its step is reached: a clear placed earlier is already skipped by the time a cancellation lands on a later step. From the end it also withdraws a thumb `Answer the request` had just posted, so a cancellation arriving during or after that step heals. The named fix -- gate the conclusion read on `cancelled()` inside the script -- is not available. `cancelled()` is readable only in a step or job `if`; PipelineTemplateEvaluator.EvaluateStepEnvironment creates its context without the expression state CancelledFunction asserts on, so `env: X: ${{ cancelled() }}` fails the step before its condition runs, and actionlint rejects the workflow. Harness: reactions.sh runs all three reaction steps against a gh stub, 38 assertions over 22 cases, including a cancelled run with the verdict outputs populated. conditions.py covers what a shell harness cannot see, 33 assertions, and states the invariant over the file as well as a table -- no step reading check_path or verdict_produced may run on a cancelled job. PLT-1166 Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/seidroid-review.yml | 101 ++++++-- .github/workflows/workflow-test-self.yml | 22 ++ test/seidroid-review/.gitignore | 5 + test/seidroid-review/README.md | 37 ++- test/seidroid-review/bin-reactions/gh | 68 ++++++ test/seidroid-review/conditions.py | 291 +++++++++++++++++++++++ test/seidroid-review/reactions.sh | 175 ++++++++++++++ 7 files changed, 677 insertions(+), 22 deletions(-) create mode 100755 test/seidroid-review/bin-reactions/gh create mode 100644 test/seidroid-review/conditions.py create mode 100755 test/seidroid-review/reactions.sh diff --git a/.github/workflows/seidroid-review.yml b/.github/workflows/seidroid-review.yml index f771477..7136c3e 100644 --- a/.github/workflows/seidroid-review.yml +++ b/.github/workflows/seidroid-review.yml @@ -58,7 +58,10 @@ run-name: UCI / seidroid review / ${{ (github.event.issue.number || github.event # surfacing steps gate on whether a verdict was produced rather than on # the exit code, so a review that reached one still posts it whatever else # went wrong; `!cancelled()` rather than `success()` keeps that true while -# still skipping a run superseded by a newer trigger. +# still skipping a run superseded by a newer trigger. Every surfacing step +# skips such a run. One step does not, and it is the last in the job: it +# withdraws the reactions a cancelled run left on the trigger comment, and +# it reads no verdict, so it can state no outcome. # - allow-tools is a workflow input, defaulting to Bash,Read because the # prompt's first step is a shell command and a declined prompt therefore # produces a turn that reports it could not read the diff. Its access @@ -1027,8 +1030,9 @@ jobs: # the same comment returns the existing reaction rather than adding a # second one, so a retry needs no cleanup. # - # `Answer the request` withdraws this reaction, on every path it takes. A - # review that keeps it reads as a review that is still running. + # `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" \ -f content=eyes >/dev/null 2>&1; then echo "acknowledged comment $TRIGGER_ID" @@ -2399,26 +2403,22 @@ jobs: # owed an answer even then. Everything here needs only GITHUB_TOKEN and the # job's issues: write. # - # The condition names two facts: a review turn, and a comment to answer. A - # close produces no verdict, so a request to tear a session down earns no - # answer. + # The condition names three facts: a review turn, not cancelled, and a comment + # to answer. A close produces no verdict, so a request to tear a session down + # earns no answer. # - # always(), where every other step in this job skips a cancelled run. A newer - # `@seidroid review` cancels the run in flight, and the eyes are on the older - # comment by then. The newer run answers its OWN comment id, so a step that - # skips on cancellation leaves that older request wearing eyes nothing clears. - # - # What always() admits is a run with no check file, and the missing-conclusion - # arm below already answers it: withdraw all three of this bot's reactions, - # post nothing. Cheap to admit. That arm spends four API calls and takes no - # position on the pull request. It starts none of the work a cancellation is - # there to stop. A runner shutdown skips every remaining condition, so nothing - # in this job survives that. + # !cancelled() is what keeps a thumb honest, and it is not interchangeable with + # always() here. A cancellation arriving after `drive` finishes leaves + # check_path and verdict_produced populated, so this step would read a real + # conclusion and thumb the request -- while `Post the verdict` below skips and + # the verdict never reaches the pull request. A thumb reads as an answer, so + # that is worse than no reaction at all. The last step of this job withdraws + # the eyes a cancelled run leaves, and it cannot reach a conclusion to post. # # No verdict_produced gate: the acknowledgement waits on this step, and a re-run # replays the trigger comment id, so a run reaching no verdict still has to clear # the eyes, and a thumb an earlier attempt left there. - if: ${{ inputs.mode == 'review' && always() + if: ${{ inputs.mode == 'review' && !cancelled() && needs.guard.outputs.comment_id != '' }} continue-on-error: true shell: bash @@ -3174,3 +3174,68 @@ jobs: echo "--- report, unposted ---" printf '%s\n' "$body" echo "--- end report ---" + + - name: Withdraw the reactions on a cancelled run + # The one step in this job that runs on a cancelled run, and the whole of what + # a cancelled run is allowed to do to the trigger comment: take this bot's own + # reactions off it and say nothing. + # + # A newer `@seidroid review` cancels the run in flight. The eyes are already on + # the older comment by then, and the newer run answers its OWN comment id, so + # nothing reads the older one again -- it would wear the eyes for good. + # + # It cannot post a thumb, and that is the point of it being a separate step + # rather than always() on the step above. It reads no check file, no + # verdict_produced and no conclusion, so there is no value a cancellation could + # leave populated that would make it state an outcome. A cancelled run publishes + # nothing, so any reaction it left would name a verdict that is not on the pull + # request. + # + # LAST in the job, deliberately. A step's condition is evaluated when the step + # is reached, so a clear placed earlier is already skipped by the time a + # cancellation lands on a later one -- and it would leave both the eyes and the + # thumb the step above had just posted. From here it also withdraws that thumb, + # so a cancellation arriving during or after `Answer the request` heals rather + # than leaving a verdict nobody published. + # + # cancelled() is only readable in a step or job `if`. It is not available in + # `env` or `run`, and a workflow carrying it there is rejected, so the job status + # cannot be handed to a shell and this decision cannot move inside one script. + if: ${{ inputs.mode == 'review' && cancelled() + && needs.guard.outputs.comment_id != '' }} + continue-on-error: true + shell: bash + env: + GH_TOKEN: ${{ github.token }} + TRIGGER_REPO: ${{ github.repository }} + TRIGGER_ID: ${{ needs.guard.outputs.comment_id }} + run: | + set -euo pipefail + # Scoped to the reacting identity, for the reason `Answer the request` states: + # a human who thumbed the request is voicing an opinion, and a cancellation is + # no licence to delete it. GITHUB_TOKEN reacts as github-actions[bot], and an + # installation token cannot ask the API which login it carries, so the login is + # named here. + me="github-actions[bot]" + # 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" \ + --paginate \ + --jq ".[] | select(.user.login == \"$me\") | \"\\(.content) \\(.id)\"")"; then + echo "::warning::could not read the reactions on comment $TRIGGER_ID in $TRIGGER_REPO; the eyes from this cancelled run may stay on it" + mine="" + fi + # Only the three this workflow ever posts. A reaction of this bot's that no + # step here chooses belongs to whatever put it there. + while read -r content rid; do + [ -n "$rid" ] || continue + case " +1 -1 eyes " in *" $content "*) ;; *) continue ;; esac + if gh api -X DELETE \ + "repos/$TRIGGER_REPO/issues/comments/$TRIGGER_ID/reactions/$rid" \ + >/dev/null; then + echo "withdrew this bot's $content from comment $TRIGGER_ID" + else + echo "::warning::could not withdraw this bot's $content from comment $TRIGGER_ID in $TRIGGER_REPO" + fi + done <<< "$mine" diff --git a/.github/workflows/workflow-test-self.yml b/.github/workflows/workflow-test-self.yml index c777717..46b8191 100644 --- a/.github/workflows/workflow-test-self.yml +++ b/.github/workflows/workflow-test-self.yml @@ -34,3 +34,25 @@ jobs: run: python3 -m pip install --quiet pyyaml - 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 + 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 reaction harness + run: test/seidroid-review/reactions.sh + # Which step runs in which job state, which is where the cancellation behaviour + # 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 diff --git a/test/seidroid-review/.gitignore b/test/seidroid-review/.gitignore index de86d63..c43180d 100644 --- a/test/seidroid-review/.gitignore +++ b/test/seidroid-review/.gitignore @@ -4,3 +4,8 @@ place.sh resolve.sh fx/gen/ out/ +# Written by reactions.sh: the three reaction steps, and one directory per case. +ack.sh +answer.sh +withdraw.sh +out-reactions/ diff --git a/test/seidroid-review/README.md b/test/seidroid-review/README.md index f0d3b7b..8e25440 100644 --- a/test/seidroid-review/README.md +++ b/test/seidroid-review/README.md @@ -1,12 +1,19 @@ -# `Place findings on the code` and `Resolve the threads this review closed` +# Workflow tests -Runs both steps of `.github/workflows/seidroid-review.yml` under `bash`, against -a `gh` stub, and checks what they posted, counted and closed. +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. ```sh -test/seidroid-review/run.sh +test/seidroid-review/run.sh # placement and thread resolution +test/seidroid-review/reactions.sh # the three reaction steps +python3 test/seidroid-review/conditions.py .github/workflows/seidroid-review.yml ``` +# `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. + 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. @@ -51,3 +58,25 @@ step has to act on. The review threads themselves are generated in `run.sh`, 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 + +`reactions.sh` runs `Acknowledge the trigger`, `Answer the request` and +`Withdraw the reactions on a cancelled run` against `bin-reactions/gh`, which keeps +the reaction list a comment carries and serves it through the step's own `jq`. Each +case reports the exact set left on the trigger comment. + +Two properties every case holds to. A human's reaction is never withdrawn. Neither is +a reaction of this bot's that no step here chooses, so a `rocket` some other workflow +left survives. + +`conditions.py` covers what a shell harness cannot see. A step condition decides which +of these steps runs in which job state, and that is where the cancellation behaviour +lives: `Answer the request` reads a conclusion and may thumb the request, so it must +skip a cancelled run, and the withdrawal step must take it. The check is stated over +the file as well as over a table -- no step that reads `check_path` or +`verdict_produced` may run on a cancelled job -- so a step added later is covered. + +It models the runner's own rule that a condition naming none of +`always`/`cancelled`/`failure`/`success` is stored as `success() && (...)`, and treats +any term it does not decide as unknown rather than as false. diff --git a/test/seidroid-review/bin-reactions/gh b/test/seidroid-review/bin-reactions/gh new file mode 100755 index 0000000..6cd9107 --- /dev/null +++ b/test/seidroid-review/bin-reactions/gh @@ -0,0 +1,68 @@ +#!/usr/bin/env bash +# gh stub for the reaction steps: keeps a reaction list, serves it through the +# step's own jq, and decides per case which calls succeed. +# +# STUB_STATE the reaction array this comment carries +# STUB_LOG one line per call +# STUB_ACTOR the login a POST reacts as +# STUB_LIST ok | FAIL +# STUB_DELETE ok | FAIL +# STUB_POST ok | FAIL +set -uo pipefail +log() { printf '%s\n' "$*" >> "$STUB_LOG"; } +[ -s "$STUB_STATE" ] || echo '[]' > "$STUB_STATE" + +verb=GET path="" filter="" ; declare -a fields=() +[ "${1:-}" = api ] && shift +while [ "$#" -gt 0 ]; do + case "$1" in + -X|--method) verb="$2"; shift 2 ;; + --jq) filter="$2"; shift 2 ;; + --paginate|--slurp) shift ;; + -f|--raw-field|--field) fields+=("$2"); shift 2 ;; + -H) shift 2 ;; + *) path="$1"; shift ;; + esac +done + +case "$verb:$path" in + GET:*/reactions) + log "CALL list" + [ "${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. + if [ -n "$filter" ]; then jq -r "$filter" < "$STUB_STATE"; else cat "$STUB_STATE"; fi + ;; + DELETE:*/reactions/*) + rid="${path##*/}" + log "CALL delete $rid" + [ "${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; } + jq --argjson rid "$rid" '[.[] | select(.id != $rid)]' < "$STUB_STATE" > "$STUB_STATE.n" + mv "$STUB_STATE.n" "$STUB_STATE" + ;; + POST:*/reactions) + content="" + for f in "${fields[@]:-}"; do case "$f" in content=*) content="${f#content=}" ;; esac; done + log "CALL post $content" + [ "${STUB_POST:-ok}" = FAIL ] && exit 1 + # Idempotent per (user, content): the API returns the reaction already there + # rather than adding a second one. + if jq -e --arg c "$content" --arg u "${STUB_ACTOR:-github-actions[bot]}" \ + 'any(.[]; .content == $c and .user.login == $u)' < "$STUB_STATE" > /dev/null; then + log " already there" + else + next="$(jq '[.[].id] | max // 0 | . + 1' < "$STUB_STATE")" + jq --arg c "$content" --arg u "${STUB_ACTOR:-github-actions[bot]}" --argjson id "$next" \ + '. + [{id: $id, content: $c, user: {login: $u}}]' < "$STUB_STATE" > "$STUB_STATE.n" + mv "$STUB_STATE.n" "$STUB_STATE" + fi + jq -c --arg c "$content" '[.[] | select(.content == $c)] | last' < "$STUB_STATE" + ;; + *) + log "CALL UNSTUBBED $verb $path" + echo "gh stub: unstubbed $verb $path" >&2 + exit 64 + ;; +esac diff --git a/test/seidroid-review/conditions.py b/test/seidroid-review/conditions.py new file mode 100644 index 0000000..f6f7769 --- /dev/null +++ b/test/seidroid-review/conditions.py @@ -0,0 +1,291 @@ +#!/usr/bin/env python3 +"""Checks which reaction step runs in which job state. + +A shell harness cannot see a step condition, and the reaction steps put their +whole cancellation behaviour there: `Answer the request` reads a conclusion and +may post a thumb, so it must not run on a cancelled run, and the withdrawal step +must. + +This models the GitHub expression engine over the subset the three conditions +use: &&, ||, !, parentheses, == and !=, single-quoted strings, the status +functions, and dotted context reads. It applies the runner's own wrapping rule -- +actions/runner, PipelineTemplateConverter.ConvertToIfCondition: + + return hasStatusFunction ? condition : $"{Success}() && ({condition})"; + +so a condition that names none of always/cancelled/failure/success skips a +cancelled run without saying so. + + conditions.py +""" +import re +import sys + +import yaml + +# step -> job state -> does it run? mode is 'review' with a comment id unless said. +# +# The cancelled column is the invariant. `Answer the request` reads check_path and +# verdict_produced, and a cancellation arriving after the driver finishes leaves both +# populated -- so a run of it on a cancelled job posts a thumb for a verdict that no +# step published. +EXPECTED = { + "Acknowledge the trigger": { + ("success", "review", "7"): True, + ("failure", "review", "7"): False, + ("cancelled", "review", "7"): False, + ("success", "review", ""): False, + ("success", "close", "7"): False, + }, + "Answer the request": { + ("success", "review", "7"): True, + ("failure", "review", "7"): True, + ("cancelled", "review", "7"): False, + ("success", "review", ""): False, + ("success", "close", "7"): False, + ("cancelled", "close", "7"): False, + }, + "Withdraw the reactions on a cancelled run": { + ("success", "review", "7"): False, + ("failure", "review", "7"): False, + ("cancelled", "review", "7"): True, + ("cancelled", "review", ""): False, + ("cancelled", "close", "7"): False, + }, +} + +# What a step is allowed to read. A step that runs on a cancelled job must not be +# able to reach a conclusion, or a cancelled run can state an outcome. +CONCLUSION_INPUTS = ("check_path", "verdict_produced") + +STATUS_FUNCS = ("always", "cancelled", "failure", "success") + +TOKEN = re.compile( + r"\s*(?:(?P'(?:[^']|'')*')" + r"|(?P&&|\|\||==|!=|!|\(|\))" + r"|(?P[A-Za-z_][A-Za-z0-9_.\-]*))" +) + + +def lex(text): + pos, out = 0, [] + while pos < len(text): + if text[pos].isspace(): + pos += 1 + continue + m = TOKEN.match(text, pos) + if not m: + raise SyntaxError(f"cannot lex at {text[pos:pos + 20]!r}") + pos = m.end() + if m.group("str") is not None: + out.append(("str", m.group("str")[1:-1].replace("''", "'"))) + elif m.group("op") is not None: + out.append(("op", m.group("op"))) + else: + out.append(("word", m.group("word"))) + return out + + +class Unknown: + """A term this model does not decide.""" + + def __repr__(self): + return "UNKNOWN" + + +UNKNOWN = Unknown() + + +class Ctx: + def __init__(self, state, mode, comment_id, lenient=False): + self.state = state + self.lenient = lenient + self.values = { + "inputs.mode": mode, + "needs.guard.outputs.comment_id": comment_id, + } + + def func(self, name): + if name == "always": + return True + if name in ("cancelled", "success", "failure"): + return self.state == ("cancelled" if name == "cancelled" else name) + raise KeyError(f"unmodelled function {name}()") + + def read(self, path): + if path not in self.values: + # The sweep at the end asks only whether a step can run on a cancelled + # job. Every other term is UNKNOWN, and the three-valued operators keep + # the answer sound without modelling contexts this file does not decide. + if self.lenient: + return UNKNOWN + raise KeyError(f"unmodelled context read {path}") + return self.values[path] + + +def truthy(value): + if value is UNKNOWN: + return UNKNOWN + if isinstance(value, bool): + return value + if isinstance(value, str): + return value != "" + return bool(value) + + +def and3(left, right): + if left is False or right is False: + return False + if left is UNKNOWN or right is UNKNOWN: + return UNKNOWN + return True + + +def or3(left, right): + if left is True or right is True: + return True + if left is UNKNOWN or right is UNKNOWN: + return UNKNOWN + return True if (left or right) else False + + +def not3(value): + return UNKNOWN if value is UNKNOWN else (not value) + + +class Parser: + def __init__(self, tokens, ctx): + self.t, self.i, self.ctx = tokens, 0, ctx + + def peek(self): + return self.t[self.i] if self.i < len(self.t) else (None, None) + + def take(self, kind=None, value=None): + k, v = self.peek() + if kind and (k != kind or (value is not None and v != value)): + raise SyntaxError(f"expected {value or kind}, found {v!r}") + self.i += 1 + return v + + def parse(self): + value = self.or_() + if self.i != len(self.t): + raise SyntaxError(f"trailing tokens at {self.t[self.i:]}") + return value + + def or_(self): + left = self.and_() + while self.peek() == ("op", "||"): + self.take() + right = self.and_() # parsed first: Python's or short-circuits + left = or3(truthy(left), truthy(right)) + return left + + def and_(self): + left = self.cmp_() + while self.peek() == ("op", "&&"): + self.take() + right = self.cmp_() + left = and3(truthy(left), truthy(right)) + return left + + def cmp_(self): + left = self.unary() + k, v = self.peek() + if k == "op" and v in ("==", "!="): + self.take() + right = self.unary() + if left is UNKNOWN or right is UNKNOWN: + return UNKNOWN + return (left == right) if v == "==" else (left != right) + return left + + def unary(self): + if self.peek() == ("op", "!"): + self.take() + return not3(truthy(self.unary())) + return self.primary() + + def primary(self): + k, v = self.peek() + if k == "op" and v == "(": + self.take() + inner = self.or_() + self.take("op", ")") + return inner + if k == "str": + return self.take() + if k == "word": + name = self.take() + if self.peek() == ("op", "("): + self.take() + self.take("op", ")") + return self.ctx.func(name) + if name in ("true", "false"): + return name == "true" + return self.ctx.read(name) + raise SyntaxError(f"unexpected {v!r}") + + +def stored_condition(raw): + """What the runner keeps as the step condition.""" + expr = " ".join(str(raw).split()) + if expr.startswith("${{") and expr.endswith("}}"): + expr = expr[3:-2].strip() + if {v for k, v in lex(expr) if k == "word"} & set(STATUS_FUNCS): + return expr + return f"success() && ({expr})" + + +def main(): + with open(sys.argv[1], encoding="utf-8") as handle: + doc = yaml.safe_load(handle) + steps = {s["name"]: s for s in doc["jobs"]["review"]["steps"] if "name" in s} + + passed = failed = 0 + + def check(label, want, got): + nonlocal passed, failed + if want == got: + passed += 1 + else: + failed += 1 + print(f" FAIL {label}: want [{want}] got [{got}]") + + for name, cases in EXPECTED.items(): + if name not in steps: + print(f" FAIL no step named {name!r} in the review job") + failed += 1 + continue + expr = stored_condition(steps[name].get("if", "success()")) + print(f"== {name}\n {expr}") + for (state, mode, cid), want in cases.items(): + got = truthy(Parser(lex(expr), Ctx(state, mode, cid)).parse()) + check(f"{name} / {state} / {mode} / id={cid or 'empty'}", want, got) + + # Whatever the table above says, no step that can reach a conclusion may run on a + # cancelled job. This is the invariant, stated over the file rather than over the + # table, so a step added later is covered too. + print("== nothing that reads a conclusion runs on a cancelled run") + for name, step in steps.items(): + expr = stored_condition(step.get("if", "success()")) + verdict = truthy( + Parser(lex(expr), Ctx("cancelled", "review", "7", lenient=True)).parse() + ) + # UNKNOWN counts as "can run": the check must not pass because a term went + # unmodelled. + runs = verdict is not False + env = " ".join(str(v) for v in (step.get("env") or {}).values()) + reads = [k for k in CONCLUSION_INPUTS if k in env] + if runs and reads: + print(f" FAIL {name} runs on a cancelled run and reads {', '.join(reads)}") + failed += 1 + else: + passed += 1 + + print(f"\nassertions: {passed} passed, {failed} failed") + return 1 if failed else 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/test/seidroid-review/reactions.sh b/test/seidroid-review/reactions.sh new file mode 100755 index 0000000..6253a29 --- /dev/null +++ b/test/seidroid-review/reactions.sh @@ -0,0 +1,175 @@ +#!/usr/bin/env bash +# Runs the three reaction steps of seidroid-review.yml under bash with a gh stub, +# and checks what each leaves on the trigger comment. +# +# The steps are read out of the workflow on every run, so a run tests what the file +# says now. `conditions.py` covers the part a shell harness cannot see: which step +# runs in which job state. +set -uo pipefail +HERE="$(cd "$(dirname "$0")" && pwd)" +REPOROOT="$(cd "$HERE/../.." && pwd)" +WORKFLOW="$REPOROOT/.github/workflows/seidroid-review.yml" + +python3 "$HERE/extract.py" "$WORKFLOW" "Acknowledge the trigger" "$HERE/ack.sh" > /dev/null || { + echo "could not read 'Acknowledge the trigger' out of $WORKFLOW"; exit 1; } +python3 "$HERE/extract.py" "$WORKFLOW" "Answer the request" "$HERE/answer.sh" > /dev/null || { + echo "could not read 'Answer the request' out of $WORKFLOW"; exit 1; } +python3 "$HERE/extract.py" "$WORKFLOW" "Withdraw the reactions on a cancelled run" \ + "$HERE/withdraw.sh" > /dev/null || { + echo "could not read the withdrawal step out of $WORKFLOW"; exit 1; } + +pass=0 fail=0 +rows=() + +BOT='github-actions[bot]' +HUMAN_ALL='[{"id":21,"content":"+1","user":{"login":"brandon"}}, + {"id":22,"content":"eyes","user":{"login":"brandon"}}, + {"id":23,"content":"-1","user":{"login":"brandon"}}]' +STALE_DOWN='[{"id":31,"content":"-1","user":{"login":"github-actions[bot]"}}, + {"id":21,"content":"+1","user":{"login":"brandon"}}]' +STALE_UP='[{"id":32,"content":"+1","user":{"login":"github-actions[bot]"}}]' +# A reaction of this bot's that no step here ever chooses. Whatever put it there owns it. +FOREIGN='[{"id":41,"content":"rocket","user":{"login":"github-actions[bot]"}}, + {"id":21,"content":"+1","user":{"login":"brandon"}}]' + +check() { # label expected actual + if [ "$2" = "$3" ]; then pass=$((pass+1)); else fail=$((fail+1)); echo " FAIL $1: want [$2] got [$3]"; fi +} + +left() { jq -r '[.[] | "\(.user.login):\(.content)"] | sort | join(" ")' < "$STUB_STATE"; } +calls() { grep -c "^CALL $1" "$STUB_LOG" || true; } + +# run_case [K=V...] +# +# "which step" is answer or withdraw: a job state decides which of them GitHub runs, +# and conditions.py checks that mapping. This runs the one that state selects. +run_case() { + local name="$1" seed="$2" which="$3" conclusion="$4" produced="$5" have_check="$6" + shift 6 + CASE="$HERE/out-reactions/$name" + rm -rf "$CASE"; mkdir -p "$CASE" + export STUB_STATE="$CASE/reactions.json"; printf '%s\n' "$seed" > "$STUB_STATE" + 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 + local check_path="" + if [ "$have_check" = yes ]; then + printf '{"conclusion":"%s","title":"t"}\n' "$conclusion" > "$CASE/check.json" + check_path="$CASE/check.json" + fi + export CHECK="$check_path" VERDICT_PRODUCED="$produced" + for kv in "$@"; do export "${kv?}"; done + + # The acknowledgement first, as the job runs it, unless a case suppresses it. Its + # calls go to a log of their own: every count below is the step under test, not the + # POST that put the eyes there. + if [ "${SKIP_ACK:-no}" = no ]; then + STUB_LOG="$CASE/ack-calls.log" ; export STUB_LOG; : > "$STUB_LOG" + # ACK_POST refuses the acknowledgement alone, so a case can start from a comment + # that never got the eyes without also refusing the answer. + STUB_POST="${ACK_POST:-${STUB_POST:-ok}}" bash "$HERE/ack.sh" > "$CASE/ack.out" 2>&1 + fi + STUB_LOG="$CASE/calls.log" ; export STUB_LOG; : > "$STUB_LOG" + bash "$HERE/$which.sh" > "$CASE/step.out" 2>&1 + echo "$?" > "$CASE/rc" + rows+=("$(printf '%-28s %-8s rc=%s list=%s del=%s post=%s left=%s' \ + "$name" "$which" "$(cat "$CASE/rc")" "$(calls list)" "$(calls delete)" \ + "$(calls post)" "$(left)")") +} + +echo "== a green review thumbs the request up ==" +run_case success '[]' answer success true yes +check "left" "$BOT:+1" "$(left)" +check "one post" 1 "$(calls post)" + +echo "== a blocking review thumbs it down ==" +run_case failure '[]' answer failure true yes +check "left" "$BOT:-1" "$(left)" + +echo "== a run that reached no verdict clears and says nothing ==" +run_case no-verdict '[]' answer failure false yes +check "left" "" "$(left)" +check "no post" 0 "$(calls post)" + +echo "== neutral earns no reaction ==" +run_case neutral '[]' answer neutral true yes +check "left" "" "$(left)" + +echo "== A CANCELLED RUN, VERDICT OUTPUTS POPULATED. The case that matters: a" +echo " cancellation after the driver finishes leaves a real conclusion on disk, and" +echo " the withdrawal step must still post nothing. ==" +run_case cancelled-after-drive '[]' withdraw success true yes +check "left" "" "$(left)" +check "no post" 0 "$(calls post)" +check "eyes gone" 0 "$(grep -c 'eyes' <<< "$(left)")" + +echo "== a cancelled run before the driver finishes ==" +run_case cancelled-early '[]' withdraw - '' no +check "left" "" "$(left)" +check "no post" 0 "$(calls post)" + +echo "== a cancelled run withdraws a thumb the answer step had already posted ==" +run_case cancelled-heals "[{\"id\":51,\"content\":\"+1\",\"user\":{\"login\":\"$BOT\"}}]" \ + withdraw success true yes +check "left" "" "$(left)" +check "no post" 0 "$(calls post)" + +echo "== a human's reaction survives every path ==" +run_case success-human "$HUMAN_ALL" answer success true yes +check "left" "brandon:+1 brandon:-1 brandon:eyes $BOT:+1" "$(left)" +run_case failure-human "$HUMAN_ALL" answer failure true yes +check "left" "brandon:+1 brandon:-1 brandon:eyes $BOT:-1" "$(left)" +run_case noverdict-human "$HUMAN_ALL" answer failure false yes +check "left" "brandon:+1 brandon:-1 brandon:eyes" "$(left)" +run_case cancelled-human "$HUMAN_ALL" withdraw success true yes +check "left" "brandon:+1 brandon:-1 brandon:eyes" "$(left)" + +echo "== a stale thumb from an earlier attempt goes, a human's stays ==" +run_case stale-thumb "$STALE_DOWN" answer success true yes +check "left" "brandon:+1 $BOT:+1" "$(left)" +run_case stale-thumb-cancelled "$STALE_DOWN" withdraw success true yes +check "left" "brandon:+1" "$(left)" +run_case stale-up-noverdict "$STALE_UP" answer failure false yes +check "left" "" "$(left)" + +echo "== a reaction no step here chooses is not this job's to withdraw ==" +run_case foreign "$FOREIGN" answer success true yes +check "left" "brandon:+1 $BOT:+1 $BOT:rocket" "$(left)" +run_case foreign-cancelled "$FOREIGN" withdraw success true yes +check "left" "brandon:+1 $BOT:rocket" "$(left)" + +echo "== a refused call warns and never fails the step ==" +run_case list-refused '[{"id":21,"content":"+1","user":{"login":"brandon"}}]' \ + answer success true yes STUB_LIST=FAIL +check "rc" 0 "$(cat "$CASE/rc")" +check "warned" 1 "$(grep -c '::warning::could not read the reactions' "$CASE/step.out")" +check "thumb still" 1 "$(calls post)" +run_case delete-refused '[]' answer success true yes STUB_DELETE=FAIL +check "rc" 0 "$(cat "$CASE/rc")" +check "eyes stay" "$BOT:+1 $BOT:eyes" "$(left)" +run_case post-refused '[]' answer success true yes STUB_POST=FAIL +check "rc" 0 "$(cat "$CASE/rc")" +check "left" "" "$(left)" +run_case list-refused-cancelled '[]' withdraw - '' no STUB_LIST=FAIL +check "rc" 0 "$(cat "$CASE/rc")" +check "warned" 1 "$(grep -c '::warning::could not read the reactions' "$CASE/step.out")" +run_case delete-refused-cancelled '[]' withdraw - '' no STUB_DELETE=FAIL +check "rc" 0 "$(cat "$CASE/rc")" +check "eyes stay" "$BOT:eyes" "$(left)" + +echo "== the acknowledgement itself refused: no eyes to clear, the answer still lands ==" +run_case ack-refused '[]' answer success true yes ACK_POST=FAIL +check "rc" 0 "$(cat "$CASE/rc")" +check "ack warned" 1 "$(grep -c '::warning::could not react to comment' "$CASE/ack.out")" +check "nothing to clear" 0 "$(calls delete)" +check "left" "$BOT:+1" "$(left)" +unset ACK_POST + +echo "== no step made a call the stub does not serve ==" +check "unstubbed calls" 0 "$(grep -rc 'CALL UNSTUBBED' "$HERE/out-reactions" 2>/dev/null | awk -F: '{s+=$2} END {print s+0}')" + +echo +printf '%s\n' "${rows[@]}" +echo +echo "assertions: $pass passed, $fail failed" +[ "$fail" -eq 0 ] From 3d46b583ca3befcada202fe8407a43914928b038 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 7 Sep 2026 13:12:58 -0700 Subject: [PATCH 4/7] fix(seidroid-review): keep a thumb that answers a published verdict A cancellation can arrive once `Post the verdict` has upserted the comment and published the check -- during thread resolution, say. The withdrawal step then deleted the thumb `Answer the request` had posted, leaving a fully published review with no reaction on the request that asked for it. That reads as never answered: this step's own defect, one window later. The step's condition now also names `steps.verdict.outcome != 'success'`. It still cannot state an outcome, and the reason is stronger than an absent input: the step contains no POST, so no code path in it adds a reaction whatever it is told. The one fact it gains is the posting step's outcome -- success, failure, cancelled or skipped -- and none of those four words names a conclusion. It says whether an answer already stands, never which answer it would be. Anything but success withdraws, so an outcome the step cannot read clears rather than leaving a thumb that stands for a verdict which may not be there. The invariant sweep now reads the whole step rather than its `env` block. An inline ${{ steps.drive.outputs.check_path }} in `run:`, `with:` or `if:` reaches the same value and passed unseen before. Two harness gaps closed. Each reaction case now derives which step runs from `conditions.py --select` instead of naming it, so the shell layer and the condition layer cannot drift. And a case declares the moment the cancellation arrived, as two job states, so a late-cancellation case proves its thumb through the answer step rather than seeding one by hand. `conditions.py` also checks that every `steps.` a reaction condition reads is a real id on an earlier step. Deleting the id, or moving the withdrawal ahead of `Post the verdict`, otherwise empties the read forever with no error. reactions.sh: 49 assertions over 26 cases. conditions.py: 39 assertions. PLT-1166 Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/seidroid-review.yml | 43 ++++-- test/seidroid-review/README.md | 36 ++++- test/seidroid-review/conditions.py | 137 ++++++++++++++---- test/seidroid-review/reactions.sh | 191 +++++++++++++++++--------- 4 files changed, 300 insertions(+), 107 deletions(-) diff --git a/.github/workflows/seidroid-review.yml b/.github/workflows/seidroid-review.yml index 7136c3e..b9a525a 100644 --- a/.github/workflows/seidroid-review.yml +++ b/.github/workflows/seidroid-review.yml @@ -2521,6 +2521,9 @@ jobs: fi - name: Post the verdict + # id: the withdrawal step at the end of this job reads this step's outcome to + # decide whether the trigger comment already carries an honest answer. + id: verdict # Post only when a real verdict was produced, and even when the drive # step above exited non-zero -- `!cancelled()` runs on any outcome # except the job itself being cancelled (e.g. superseded by a newer @@ -3184,25 +3187,41 @@ jobs: # the older comment by then, and the newer run answers its OWN comment id, so # nothing reads the older one again -- it would wear the eyes for good. # - # It cannot post a thumb, and that is the point of it being a separate step - # rather than always() on the step above. It reads no check file, no - # verdict_produced and no conclusion, so there is no value a cancellation could - # leave populated that would make it state an outcome. A cancelled run publishes - # nothing, so any reaction it left would name a verdict that is not on the pull - # request. + # THIS STEP HAS NO POST. It withdraws, and there is no code path in it that adds + # a reaction, so nothing it is told can make it state an outcome. That is what + # makes the withdrawal a step of its own rather than always() on `Answer the + # request`: that step chooses a reaction from a conclusion, and a cancellation + # arriving after the driver finishes leaves check_path and verdict_produced + # populated -- so it would read a real conclusion and thumb a review that + # published nothing. # - # LAST in the job, deliberately. A step's condition is evaluated when the step - # is reached, so a clear placed earlier is already skipped by the time a + # `Post the verdict` decides whether there is anything to withdraw, and the + # driver does not. A cancellation can arrive once the verdict is already on the + # pull request -- during thread resolution, say -- and the thumb `Answer the + # request` posted answers it correctly. Withdrawing it there leaves a published + # review with no reaction on the request that asked for it, which reads as never + # answered. That is this step's own defect, one window later. + # + # The posting step's OUTCOME is what separates the two, and it is four words + # about another step: success, failure, cancelled or skipped. None of them names + # a conclusion. It says whether an answer already stands, never which answer it + # would be, so reading it cannot give this step an outcome to state. + # + # Anything but success withdraws. An outcome this step cannot read therefore + # clears, rather than leaving a thumb that stands for a verdict which may not be + # on the pull request. + # + # LAST in the job, deliberately. The runner evaluates a step's condition when it + # reaches the step, so a clear placed earlier is already skipped once a # cancellation lands on a later one -- and it would leave both the eyes and the - # thumb the step above had just posted. From here it also withdraws that thumb, - # so a cancellation arriving during or after `Answer the request` heals rather - # than leaving a verdict nobody published. + # thumb the step above had just posted. # # cancelled() is only readable in a step or job `if`. It is not available in # `env` or `run`, and a workflow carrying it there is rejected, so the job status # cannot be handed to a shell and this decision cannot move inside one script. if: ${{ inputs.mode == 'review' && cancelled() - && needs.guard.outputs.comment_id != '' }} + && needs.guard.outputs.comment_id != '' + && steps.verdict.outcome != 'success' }} continue-on-error: true shell: bash env: diff --git a/test/seidroid-review/README.md b/test/seidroid-review/README.md index 8e25440..620ca87 100644 --- a/test/seidroid-review/README.md +++ b/test/seidroid-review/README.md @@ -66,17 +66,39 @@ foreign account, no marker, and a marker quoted mid-body. the reaction list a comment carries and serves it through the step's own `jq`. Each case reports the exact set left on the trigger comment. -Two properties every case holds to. A human's reaction is never withdrawn. Neither is +No case names the step it runs. `conditions.py --select` names it, from the job state +and from `Post the verdict`'s outcome, so the two harnesses cannot drift and a case +cannot quietly stop exercising the step it claims to. + +Each case declares **two** job states, because a cancellation has a moment: the state +the runner reached `Answer the request` in, and the state it reached the withdrawal step +in. `success>cancelled` is a cancellation that arrived after the answer. The answer step +then posts its own thumb, so a late-cancellation case proves the outcome through the +steps rather than placing a reaction by hand. + +Three properties every case holds to. A human's reaction is never withdrawn. Neither is a reaction of this bot's that no step here chooses, so a `rocket` some other workflow -left survives. +left survives. And a thumb that answers a verdict already on the pull request survives a +later cancellation — withdrawing it would read as never answered. + +# The step conditions `conditions.py` covers what a shell harness cannot see. A step condition decides which -of these steps runs in which job state, and that is where the cancellation behaviour -lives: `Answer the request` reads a conclusion and may thumb the request, so it must -skip a cancelled run, and the withdrawal step must take it. The check is stated over -the file as well as over a table -- no step that reads `check_path` or -`verdict_produced` may run on a cancelled job -- so a step added later is covered. +reaction step runs in which job state, and that is where the cancellation behaviour +lives. `Answer the request` reads a conclusion and may thumb the request, so it must +skip a cancelled run. The withdrawal step must take a cancelled run, unless +`Post the verdict` already completed. It models the runner's own rule that a condition naming none of `always`/`cancelled`/`failure`/`success` is stored as `success() && (...)`, and treats any term it does not decide as unknown rather than as false. + +Two checks are stated over the file rather than over a table, so a step added later is +covered: + +- No step that can run on a cancelled job may reach `check_path` or + `verdict_produced`. The haystack is the whole step, not its `env` block: an inline + `${{ steps.drive.outputs.check_path }}` in `run:`, `with:` or `if:` reaches the same + value. +- Every `steps.` a reaction step's condition reads must be a real id on an earlier + step. Delete the id and the read is empty forever, with no error anywhere. diff --git a/test/seidroid-review/conditions.py b/test/seidroid-review/conditions.py index f6f7769..8156ae6 100644 --- a/test/seidroid-review/conditions.py +++ b/test/seidroid-review/conditions.py @@ -17,6 +17,11 @@ cancelled run without saying so. conditions.py + conditions.py --select + +--select names the reaction steps GitHub runs in that state, in job order. +reactions.sh uses it, so no case there hardcodes which step a state selects and the +two harnesses cannot drift. """ import re import sys @@ -29,33 +34,53 @@ # verdict_produced, and a cancellation arriving after the driver finishes leaves both # populated -- so a run of it on a cancelled job posts a thumb for a verdict that no # step published. +# step -> (job state, mode, comment id, `Post the verdict` outcome) -> does it run? +# +# Two invariants live here. `Answer the request` reads check_path and verdict_produced, +# and a cancellation arriving after the driver finishes leaves both populated -- so a +# run of it on a cancelled job posts a thumb for a verdict no step published. And the +# withdrawal must not strip a thumb that answers a verdict which DID publish: a +# cancellation can arrive after `Post the verdict` upserted the comment, and the +# trigger comment then carries an honest answer. EXPECTED = { "Acknowledge the trigger": { - ("success", "review", "7"): True, - ("failure", "review", "7"): False, - ("cancelled", "review", "7"): False, - ("success", "review", ""): False, - ("success", "close", "7"): False, + ("success", "review", "7", ""): True, + ("failure", "review", "7", ""): False, + ("cancelled", "review", "7", ""): False, + ("success", "review", "", ""): False, + ("success", "close", "7", ""): False, }, "Answer the request": { - ("success", "review", "7"): True, - ("failure", "review", "7"): True, - ("cancelled", "review", "7"): False, - ("success", "review", ""): False, - ("success", "close", "7"): False, - ("cancelled", "close", "7"): False, + ("success", "review", "7", ""): True, + ("failure", "review", "7", ""): True, + ("cancelled", "review", "7", ""): False, + ("cancelled", "review", "7", "success"): False, + ("success", "review", "", ""): False, + ("success", "close", "7", ""): False, + ("cancelled", "close", "7", ""): False, }, "Withdraw the reactions on a cancelled run": { - ("success", "review", "7"): False, - ("failure", "review", "7"): False, - ("cancelled", "review", "7"): True, - ("cancelled", "review", ""): False, - ("cancelled", "close", "7"): False, + # Nothing to withdraw on a run that was not cancelled. + ("success", "review", "7", "success"): False, + ("failure", "review", "7", "failure"): False, + # Cancelled before the verdict published: withdraw. + ("cancelled", "review", "7", "skipped"): True, + ("cancelled", "review", "7", "cancelled"): True, + ("cancelled", "review", "7", "failure"): True, + # An outcome this step cannot read clears rather than leaving a thumb. + ("cancelled", "review", "7", ""): True, + # THE CASE FOR THE LATE WINDOW. The verdict is on the pull request and the + # thumb answers it, so the thumb stays. + ("cancelled", "review", "7", "success"): False, + ("cancelled", "review", "", "skipped"): False, + ("cancelled", "close", "7", "skipped"): False, }, } -# What a step is allowed to read. A step that runs on a cancelled job must not be -# able to reach a conclusion, or a cancelled run can state an outcome. +# What a step must not be able to reach when it runs on a cancelled job, or a cancelled +# run can state an outcome. Matched against the WHOLE step rather than its `env` block: +# an inline ${{ steps.drive.outputs.check_path }} in `run:`, `with:` or `if:` reaches +# the same value and would otherwise pass unseen. CONCLUSION_INPUTS = ("check_path", "verdict_produced") STATUS_FUNCS = ("always", "cancelled", "failure", "success") @@ -97,12 +122,15 @@ def __repr__(self): class Ctx: - def __init__(self, state, mode, comment_id, lenient=False): + def __init__(self, state, mode, comment_id, verdict_outcome="", lenient=False): self.state = state self.lenient = lenient self.values = { "inputs.mode": mode, "needs.guard.outputs.comment_id": comment_id, + # One of success, failure, cancelled, skipped, or empty for a step that + # never reported. Four words about another step; none of them a conclusion. + "steps.verdict.outcome": verdict_outcome, } def func(self, name): @@ -237,7 +265,35 @@ def stored_condition(raw): return f"success() && ({expr})" +REACTION_STEPS = ( + "Acknowledge the trigger", + "Answer the request", + "Withdraw the reactions on a cancelled run", +) + + +def load(path): + with open(path, encoding="utf-8") as handle: + return yaml.safe_load(handle) + + +def select(argv): + """Print the reaction steps that run, one per line, in job order.""" + workflow, state, mode, cid, verdict_outcome = argv + steps = {s["name"]: s for s in load(workflow)["jobs"]["review"]["steps"] if "name" in s} + for name in REACTION_STEPS: + if name not in steps: + continue + expr = stored_condition(steps[name].get("if", "success()")) + ctx = Ctx(state, mode, cid, "" if verdict_outcome == "-" else verdict_outcome) + if truthy(Parser(lex(expr), ctx).parse()) is True: + print(name) + return 0 + + def main(): + if sys.argv[1] == "--select": + return select(sys.argv[2:]) with open(sys.argv[1], encoding="utf-8") as handle: doc = yaml.safe_load(handle) steps = {s["name"]: s for s in doc["jobs"]["review"]["steps"] if "name" in s} @@ -259,9 +315,38 @@ def check(label, want, got): continue expr = stored_condition(steps[name].get("if", "success()")) print(f"== {name}\n {expr}") - for (state, mode, cid), want in cases.items(): - got = truthy(Parser(lex(expr), Ctx(state, mode, cid)).parse()) - check(f"{name} / {state} / {mode} / id={cid or 'empty'}", want, got) + for (state, mode, cid, vo), want in cases.items(): + got = truthy(Parser(lex(expr), Ctx(state, mode, cid, vo)).parse()) + check( + f"{name} / {state} / {mode} / id={cid or 'empty'}" + f" / verdict={vo or 'unreported'}", + want, + got, + ) + + # A `steps..outcome` read only works when that id exists and belongs to an + # EARLIER step. Delete the id and the read is silently empty forever, which sends + # the withdrawal down its clearing path on every cancellation -- including the one + # where the verdict published and the thumb is honest. The model takes the outcome + # as an argument, so nothing above would notice. + print("== every steps. a reaction step reads exists, and runs before it") + order = [s.get("name") for s in doc["jobs"]["review"]["steps"]] + ids = { + s["id"]: i + for i, s in enumerate(doc["jobs"]["review"]["steps"]) + if s.get("id") + } + for name in EXPECTED: + if name not in steps: + continue + reader = order.index(name) + for ref in sorted(set(re.findall(r"steps\.([A-Za-z0-9_-]+)\.", str(steps[name].get("if", ""))))): + if ref not in ids: + check(f"{name} reads steps.{ref}, which is no step's id", True, False) + elif ids[ref] > reader: + check(f"{name} reads steps.{ref}, which runs later", True, False) + else: + check(f"{name} reads steps.{ref}", True, True) # Whatever the table above says, no step that can reach a conclusion may run on a # cancelled job. This is the invariant, stated over the file rather than over the @@ -270,13 +355,15 @@ def check(label, want, got): for name, step in steps.items(): expr = stored_condition(step.get("if", "success()")) verdict = truthy( - Parser(lex(expr), Ctx("cancelled", "review", "7", lenient=True)).parse() + Parser( + lex(expr), Ctx("cancelled", "review", "7", lenient=True) + ).parse() ) # UNKNOWN counts as "can run": the check must not pass because a term went # unmodelled. runs = verdict is not False - env = " ".join(str(v) for v in (step.get("env") or {}).values()) - reads = [k for k in CONCLUSION_INPUTS if k in env] + haystack = yaml.safe_dump(step, default_flow_style=False) + reads = [k for k in CONCLUSION_INPUTS if k in haystack] if runs and reads: print(f" FAIL {name} runs on a cancelled run and reads {', '.join(reads)}") failed += 1 diff --git a/test/seidroid-review/reactions.sh b/test/seidroid-review/reactions.sh index 6253a29..fdbdb4c 100755 --- a/test/seidroid-review/reactions.sh +++ b/test/seidroid-review/reactions.sh @@ -1,27 +1,39 @@ #!/usr/bin/env bash -# Runs the three reaction steps of seidroid-review.yml under bash with a gh stub, -# and checks what each leaves on the trigger comment. +# Runs the reaction steps of seidroid-review.yml under bash with a gh stub, and checks +# what each case leaves on the trigger comment. # -# The steps are read out of the workflow on every run, so a run tests what the file -# says now. `conditions.py` covers the part a shell harness cannot see: which step -# runs in which job state. +# No case names the step it runs. `conditions.py --select` names it, from the job state +# and from `Post the verdict`'s outcome, so the shell layer and the condition layer +# cannot drift and a case cannot quietly stop exercising the step it claims to. set -uo pipefail HERE="$(cd "$(dirname "$0")" && pwd)" REPOROOT="$(cd "$HERE/../.." && pwd)" WORKFLOW="$REPOROOT/.github/workflows/seidroid-review.yml" +SELECT="python3 $HERE/conditions.py --select $WORKFLOW" -python3 "$HERE/extract.py" "$WORKFLOW" "Acknowledge the trigger" "$HERE/ack.sh" > /dev/null || { - echo "could not read 'Acknowledge the trigger' out of $WORKFLOW"; exit 1; } -python3 "$HERE/extract.py" "$WORKFLOW" "Answer the request" "$HERE/answer.sh" > /dev/null || { - echo "could not read 'Answer the request' out of $WORKFLOW"; exit 1; } -python3 "$HERE/extract.py" "$WORKFLOW" "Withdraw the reactions on a cancelled run" \ - "$HERE/withdraw.sh" > /dev/null || { - echo "could not read the withdrawal step out of $WORKFLOW"; exit 1; } +extract() { # step name, output file + python3 "$HERE/extract.py" "$WORKFLOW" "$1" "$2" > /dev/null || { + echo "could not read '$1' out of $WORKFLOW"; exit 1; } +} +extract "Acknowledge the trigger" "$HERE/ack.sh" +extract "Answer the request" "$HERE/answer.sh" +extract "Withdraw the reactions on a cancelled run" "$HERE/withdraw.sh" + +# Step name -> the file it was extracted to. +script_for() { + case "$1" in + "Acknowledge the trigger") echo "$HERE/ack.sh" ;; + "Answer the request") echo "$HERE/answer.sh" ;; + "Withdraw the reactions on a cancelled run") echo "$HERE/withdraw.sh" ;; + *) echo "" ;; + esac +} pass=0 fail=0 rows=() BOT='github-actions[bot]' +NONE='[]' HUMAN_ALL='[{"id":21,"content":"+1","user":{"login":"brandon"}}, {"id":22,"content":"eyes","user":{"login":"brandon"}}, {"id":23,"content":"-1","user":{"login":"brandon"}}]' @@ -37,15 +49,21 @@ check() { # label expected actual } left() { jq -r '[.[] | "\(.user.login):\(.content)"] | sort | join(" ")' < "$STUB_STATE"; } -calls() { grep -c "^CALL $1" "$STUB_LOG" || true; } +calls() { grep -c "^CALL $1" "$CASE/calls.log" || true; } +ran() { grep -c "^$1\$" "$CASE/ran.txt" || true; } -# run_case [K=V...] +# run_case \ +# [K=V...] # -# "which step" is answer or withdraw: a job state decides which of them GitHub runs, -# and conditions.py checks that mapping. This runs the one that state selects. +# Two states, because a cancellation has a moment. `answer state` is the job state when +# the runner reached `Answer the request`; `final state` is the state when it reached the +# withdrawal step at the end. A cancellation arriving after the verdict published gives +# success then cancelled, so the answer step posts its thumb and the fixture does not +# have to place one. run_case() { - local name="$1" seed="$2" which="$3" conclusion="$4" produced="$5" have_check="$6" - shift 6 + local name="$1" seed="$2" answer_state="$3" state="$4" verdict_outcome="$5" + local conclusion="$6" produced="$7" have_check="$8" + shift 8 CASE="$HERE/out-reactions/$name" rm -rf "$CASE"; mkdir -p "$CASE" export STUB_STATE="$CASE/reactions.json"; printf '%s\n' "$seed" > "$STUB_STATE" @@ -59,114 +77,161 @@ run_case() { fi export CHECK="$check_path" VERDICT_PRODUCED="$produced" for kv in "$@"; do export "${kv?}"; done - - # The acknowledgement first, as the job runs it, unless a case suppresses it. Its - # calls go to a log of their own: every count below is the step under test, not the - # POST that put the eyes there. - if [ "${SKIP_ACK:-no}" = no ]; then - STUB_LOG="$CASE/ack-calls.log" ; export STUB_LOG; : > "$STUB_LOG" + : > "$CASE/ran.txt" + + # The acknowledgement runs before any cancellation could land, so it is selected in + # the state the job starts in, not in the state it ends in. + local ack_step + ack_step="$($SELECT success review 7 - | grep '^Acknowledge the trigger$' || true)" + if [ -n "$ack_step" ] && [ "${SKIP_ACK:-no}" = no ]; then + STUB_LOG="$CASE/ack-calls.log"; export STUB_LOG; : > "$STUB_LOG" # ACK_POST refuses the acknowledgement alone, so a case can start from a comment # that never got the eyes without also refusing the answer. STUB_POST="${ACK_POST:-${STUB_POST:-ok}}" bash "$HERE/ack.sh" > "$CASE/ack.out" 2>&1 + echo "$ack_step" >> "$CASE/ran.txt" fi - STUB_LOG="$CASE/calls.log" ; export STUB_LOG; : > "$STUB_LOG" - bash "$HERE/$which.sh" > "$CASE/step.out" 2>&1 - echo "$?" > "$CASE/rc" - rows+=("$(printf '%-28s %-8s rc=%s list=%s del=%s post=%s left=%s' \ - "$name" "$which" "$(cat "$CASE/rc")" "$(calls list)" "$(calls delete)" \ - "$(calls post)" "$(left)")") + + STUB_LOG="$CASE/calls.log"; export STUB_LOG; : > "$STUB_LOG" + : > "$CASE/step.out" + local rc=0 step script + run_selected() { # state, verdict outcome, step to keep + while IFS= read -r step; do + [ "$step" = "$3" ] || continue + script="$(script_for "$step")" + if [ -z "$script" ]; then echo "no script for step '$step'"; exit 1; fi + bash "$script" >> "$CASE/step.out" 2>&1 || rc=$? + echo "$step" >> "$CASE/ran.txt" + done < <($SELECT "$1" review 7 "$2") + } + run_selected "$answer_state" "$verdict_outcome" "Answer the request" + run_selected "$state" "$verdict_outcome" "Withdraw the reactions on a cancelled run" + echo "$rc" > "$CASE/rc" + + rows+=("$(printf '%-29s %-19s verdict=%-10s ran=%-9s list=%s del=%s post=%s left=%s' \ + "$name" "$answer_state>$state" "${verdict_outcome/-/unreported}" \ + "$(sed -n 's/^Answer the request$/answer/p;s/^Withdraw.*/withdraw/p' \ + "$CASE/ran.txt" | paste -sd+ - || true)" \ + "$(calls list)" "$(calls delete)" "$(calls post)" "$(left)")") } echo "== a green review thumbs the request up ==" -run_case success '[]' answer success true yes +run_case success "$NONE" success success success success true yes +check "answer ran" 1 "$(ran 'Answer the request')" check "left" "$BOT:+1" "$(left)" -check "one post" 1 "$(calls post)" echo "== a blocking review thumbs it down ==" -run_case failure '[]' answer failure true yes +run_case failure "$NONE" success success success failure true yes check "left" "$BOT:-1" "$(left)" echo "== a run that reached no verdict clears and says nothing ==" -run_case no-verdict '[]' answer failure false yes +run_case no-verdict "$NONE" success success skipped failure false yes check "left" "" "$(left)" check "no post" 0 "$(calls post)" echo "== neutral earns no reaction ==" -run_case neutral '[]' answer neutral true yes +run_case neutral "$NONE" success success skipped neutral true yes check "left" "" "$(left)" -echo "== A CANCELLED RUN, VERDICT OUTPUTS POPULATED. The case that matters: a" -echo " cancellation after the driver finishes leaves a real conclusion on disk, and" -echo " the withdrawal step must still post nothing. ==" -run_case cancelled-after-drive '[]' withdraw success true yes -check "left" "" "$(left)" -check "no post" 0 "$(calls post)" -check "eyes gone" 0 "$(grep -c 'eyes' <<< "$(left)")" +echo "== a cancelled run with the verdict outputs POPULATED still posts nothing." +echo " A cancellation after the driver finishes leaves a real conclusion on disk. ==" +run_case cancelled-after-drive "$NONE" cancelled cancelled skipped success true yes +check "withdraw ran" 1 "$(ran 'Withdraw the reactions on a cancelled run')" +check "answer skipped" 0 "$(ran 'Answer the request')" +check "left" "" "$(left)" +check "no post" 0 "$(calls post)" echo "== a cancelled run before the driver finishes ==" -run_case cancelled-early '[]' withdraw - '' no +run_case cancelled-early "$NONE" cancelled cancelled skipped - '' no check "left" "" "$(left)" check "no post" 0 "$(calls post)" -echo "== a cancelled run withdraws a thumb the answer step had already posted ==" -run_case cancelled-heals "[{\"id\":51,\"content\":\"+1\",\"user\":{\"login\":\"$BOT\"}}]" \ - withdraw success true yes +echo "== cancelled while the verdict was posting: the thumb goes, it may not have landed ==" +run_case cancelled-mid-publish "$NONE" success cancelled cancelled success true yes +check "withdraw ran" 1 "$(ran 'Withdraw the reactions on a cancelled run')" check "left" "" "$(left)" -check "no post" 0 "$(calls post)" + +echo "== the verdict failed to post: the thumb goes, it would stand for nothing ==" +run_case cancelled-publish-failed "$NONE" success cancelled failure success true yes +check "left" "" "$(left)" + +echo "== an outcome this step cannot read clears rather than leaving a thumb ==" +run_case cancelled-unreported "$NONE" success cancelled - success true yes +check "withdraw ran" 1 "$(ran 'Withdraw the reactions on a cancelled run')" +check "left" "" "$(left)" + +echo "== CANCELLED AFTER THE VERDICT PUBLISHED. The thumb answers a review that is on" +echo " the pull request, so it survives: withdrawing it would read as never answered. ==" +run_case cancelled-after-publish "$NONE" success cancelled success success true yes +check "answer ran" 1 "$(ran 'Answer the request')" +check "withdraw skipped" 0 "$(ran 'Withdraw the reactions on a cancelled run')" +check "one post, no later delete" "1 1" "$(calls post) $(calls delete)" +check "THUMB SURVIVES" "$BOT:+1" "$(left)" + +echo "== and it survives beside a human's reactions ==" +run_case cancelled-after-publish-human \ + '[{"id":21,"content":"-1","user":{"login":"brandon"}}]' \ + success cancelled success success true yes +check "left" "brandon:-1 $BOT:+1" "$(left)" echo "== a human's reaction survives every path ==" -run_case success-human "$HUMAN_ALL" answer success true yes +run_case success-human "$HUMAN_ALL" success success success success true yes check "left" "brandon:+1 brandon:-1 brandon:eyes $BOT:+1" "$(left)" -run_case failure-human "$HUMAN_ALL" answer failure true yes +run_case failure-human "$HUMAN_ALL" success success success failure true yes check "left" "brandon:+1 brandon:-1 brandon:eyes $BOT:-1" "$(left)" -run_case noverdict-human "$HUMAN_ALL" answer failure false yes +run_case noverdict-human "$HUMAN_ALL" success success skipped failure false yes check "left" "brandon:+1 brandon:-1 brandon:eyes" "$(left)" -run_case cancelled-human "$HUMAN_ALL" withdraw success true yes +run_case cancelled-human "$HUMAN_ALL" cancelled cancelled skipped success true yes check "left" "brandon:+1 brandon:-1 brandon:eyes" "$(left)" echo "== a stale thumb from an earlier attempt goes, a human's stays ==" -run_case stale-thumb "$STALE_DOWN" answer success true yes +run_case stale-thumb "$STALE_DOWN" success success success success true yes check "left" "brandon:+1 $BOT:+1" "$(left)" -run_case stale-thumb-cancelled "$STALE_DOWN" withdraw success true yes +run_case stale-thumb-cancelled "$STALE_DOWN" cancelled cancelled skipped success true yes check "left" "brandon:+1" "$(left)" -run_case stale-up-noverdict "$STALE_UP" answer failure false yes +run_case stale-up-noverdict "$STALE_UP" success success skipped failure false yes check "left" "" "$(left)" echo "== a reaction no step here chooses is not this job's to withdraw ==" -run_case foreign "$FOREIGN" answer success true yes +run_case foreign "$FOREIGN" success success success success true yes check "left" "brandon:+1 $BOT:+1 $BOT:rocket" "$(left)" -run_case foreign-cancelled "$FOREIGN" withdraw success true yes +run_case foreign-cancelled "$FOREIGN" cancelled cancelled skipped success true yes check "left" "brandon:+1 $BOT:rocket" "$(left)" echo "== a refused call warns and never fails the step ==" run_case list-refused '[{"id":21,"content":"+1","user":{"login":"brandon"}}]' \ - answer success true yes STUB_LIST=FAIL + success success success success true yes STUB_LIST=FAIL check "rc" 0 "$(cat "$CASE/rc")" check "warned" 1 "$(grep -c '::warning::could not read the reactions' "$CASE/step.out")" check "thumb still" 1 "$(calls post)" -run_case delete-refused '[]' answer success true yes STUB_DELETE=FAIL +run_case delete-refused "$NONE" success success success success true yes STUB_DELETE=FAIL check "rc" 0 "$(cat "$CASE/rc")" check "eyes stay" "$BOT:+1 $BOT:eyes" "$(left)" -run_case post-refused '[]' answer success true yes STUB_POST=FAIL +run_case post-refused "$NONE" success success success success true yes STUB_POST=FAIL check "rc" 0 "$(cat "$CASE/rc")" check "left" "" "$(left)" -run_case list-refused-cancelled '[]' withdraw - '' no STUB_LIST=FAIL +run_case list-refused-cancelled "$NONE" cancelled cancelled skipped - '' no STUB_LIST=FAIL check "rc" 0 "$(cat "$CASE/rc")" check "warned" 1 "$(grep -c '::warning::could not read the reactions' "$CASE/step.out")" -run_case delete-refused-cancelled '[]' withdraw - '' no STUB_DELETE=FAIL +run_case delete-refused-cancelled "$NONE" cancelled cancelled skipped - '' no STUB_DELETE=FAIL check "rc" 0 "$(cat "$CASE/rc")" check "eyes stay" "$BOT:eyes" "$(left)" echo "== the acknowledgement itself refused: no eyes to clear, the answer still lands ==" -run_case ack-refused '[]' answer success true yes ACK_POST=FAIL +run_case ack-refused "$NONE" success success success success true yes ACK_POST=FAIL check "rc" 0 "$(cat "$CASE/rc")" check "ack warned" 1 "$(grep -c '::warning::could not react to comment' "$CASE/ack.out")" check "nothing to clear" 0 "$(calls delete)" check "left" "$BOT:+1" "$(left)" unset ACK_POST +echo "== a close reacts nowhere, so nothing is left to clear ==" +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 skipped | paste -sd, -)" + echo "== no step made a call the stub does not serve ==" -check "unstubbed calls" 0 "$(grep -rc 'CALL UNSTUBBED' "$HERE/out-reactions" 2>/dev/null | awk -F: '{s+=$2} END {print s+0}')" +check "unstubbed calls" 0 \ + "$(grep -rh 'CALL UNSTUBBED' "$HERE/out-reactions" 2>/dev/null | wc -l | tr -d ' ')" echo printf '%s\n' "${rows[@]}" From cd111a3e69ea520f07ac705dd41a1ff0e5e97532 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 7 Sep 2026 13:28:09 -0700 Subject: [PATCH 5/7] fix(seidroid-review): leave an earlier run's thumb where its verdict still stands A re-run replays the trigger comment id, so the comment can already carry a thumb from an earlier run whose verdict IS on the pull request. A re-run cancelled before it answered took that thumb along with its own eyes, leaving the comment bare while the verdict it asked for still stood. That is the "reads as never answered" failure, one run earlier. `Answer the request` gains `id: answer`, and the withdrawal reads its outcome to decide what this run may take. skipped is the only value meaning "never touched the comment", and there the withdrawal takes the eyes alone. Once the answer step has run, every reaction on the comment is this run's own, and this run published nothing, so all three go. failure, cancelled and an unreadable value all clear: a thumb standing for a verdict nobody published is the worse of the two wrongs. The structural property holds. The step still has no POST, and an outcome is four words about another step with no conclusion among them. One case is left over and the comment now records it rather than claiming the invariant outright. A run that answers, withdrawing an earlier thumb and posting its own, and is then cancelled before publishing, leaves the comment bare. The earlier thumb is already gone by then, so nothing here can restore it. Both file-level checks now walk every job's RAW steps list and search the whole step. Keyed off a name they dropped an unnamed step, and `- uses: ...` with no `name:` is the usual shape -- the step most likely to arrive later was the one they could not see. The id check reached only `if`, so the withdrawal's new `env` read would have gone unchecked. Also removes a stale duplicate of the EXPECTED header whose key description no longer matched, and clears each per-case harness knob at the top of run_case: an ANSWERED_AS override leaked into every later case and silently disarmed one. reactions.sh: 60 assertions over 32 cases. conditions.py: 77 assertions. PLT-1166 Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/seidroid-review.yml | 46 ++++++++++- test/seidroid-review/README.md | 26 +++--- test/seidroid-review/conditions.py | 113 ++++++++++++++------------ test/seidroid-review/reactions.sh | 56 ++++++++++++- 4 files changed, 173 insertions(+), 68 deletions(-) diff --git a/.github/workflows/seidroid-review.yml b/.github/workflows/seidroid-review.yml index b9a525a..c2685d7 100644 --- a/.github/workflows/seidroid-review.yml +++ b/.github/workflows/seidroid-review.yml @@ -2394,6 +2394,9 @@ jobs: fi - name: Answer the request + # id: the withdrawal step at the end of this job reads this step's outcome to + # tell a run that already touched the trigger comment from one that never did. + id: answer # The verdict, on the comment that asked for it, so the person who asked reads # the outcome where they asked. The eyes at the top of this job say it started; # this step withdraws them and says how it ended. @@ -3205,12 +3208,23 @@ jobs: # The posting step's OUTCOME is what separates the two, and it is four words # about another step: success, failure, cancelled or skipped. None of them names # a conclusion. It says whether an answer already stands, never which answer it - # would be, so reading it cannot give this step an outcome to state. + # would be, so reading it cannot give this step an outcome to state. The same + # holds for the answer step's outcome, which the withdrawal set below reads. # # Anything but success withdraws. An outcome this step cannot read therefore # clears, rather than leaving a thumb that stands for a verdict which may not be # on the pull request. # + # WHAT IT PROTECTS, AND WHERE THAT STOPS. A thumb survives when the verdict it + # answers is on the pull request AND this run is the run that posted it, or when + # this run never reached the answer step at all. One case is left over: this run + # answers, which withdraws an earlier run's thumb and posts its own, and is then + # cancelled before publishing. The comment ends bare while the earlier run's + # verdict still stands. The earlier thumb is already gone by then -- the answer + # step took it -- so nothing here can put it back, and knowing it happened would + # take a read of the pull request this step deliberately does not make. A + # cancellation lands during the driver far more often than in that window. + # # LAST in the job, deliberately. The runner evaluates a step's condition when it # reaches the step, so a clear placed earlier is already skipped once a # cancellation lands on a later one -- and it would leave both the eyes and the @@ -3228,6 +3242,9 @@ jobs: GH_TOKEN: ${{ github.token }} TRIGGER_REPO: ${{ github.repository }} TRIGGER_ID: ${{ needs.guard.outputs.comment_id }} + # Whether this run ever reached the step that answers. See the withdrawal set + # below. Four words about another step; none of them a conclusion. + ANSWERED: ${{ steps.answer.outcome }} run: | set -euo pipefail # Scoped to the reacting identity, for the reason `Answer the request` states: @@ -3236,6 +3253,27 @@ jobs: # installation token cannot ask the API which login it carries, so the login is # named here. me="github-actions[bot]" + # WHAT THIS RUN MAY TAKE, and it is not always all three. + # + # A re-run replays the trigger comment id, so the comment can already carry a + # thumb from an earlier run whose verdict IS on the pull request. `Answer the + # request` withdraws that thumb and posts this run's own, so once it has run, + # every reaction on the comment belongs to this run and this run published + # nothing -- take all three. + # + # A run cancelled before it reached that step has posted only the eyes. A thumb + # there answers an earlier run, and taking it leaves that run's published + # verdict with no reaction on the request, which reads as never answered. + # + # skipped is the only value that means "never touched the comment". failure and + # cancelled both mean the step ran partway and most likely cleared the earlier + # thumb already, so they clear, and so does a value this step cannot read: a + # thumb this run posted for a verdict nobody published is the worse of the two + # wrongs. + case "${ANSWERED:-}" in + skipped) takeable=" eyes " ;; + *) takeable=" +1 -1 eyes " ;; + esac # Listed into a variable and read from it rather than through a pipe, so # nothing this block reports can be read back as a reaction id. Paginated, # because a busy comment carries more reactions than one page holds. @@ -3245,11 +3283,11 @@ jobs: echo "::warning::could not read the reactions on comment $TRIGGER_ID in $TRIGGER_REPO; the eyes from this cancelled run may stay on it" mine="" fi - # Only the three this workflow ever posts. A reaction of this bot's that no - # step here chooses belongs to whatever put it there. + # Only what this run may take. A reaction of this bot's outside that set + # belongs to whatever put it there. while read -r content rid; do [ -n "$rid" ] || continue - case " +1 -1 eyes " in *" $content "*) ;; *) continue ;; esac + case "$takeable" in *" $content "*) ;; *) continue ;; esac if gh api -X DELETE \ "repos/$TRIGGER_REPO/issues/comments/$TRIGGER_ID/reactions/$rid" \ >/dev/null; then diff --git a/test/seidroid-review/README.md b/test/seidroid-review/README.md index 620ca87..5130d6f 100644 --- a/test/seidroid-review/README.md +++ b/test/seidroid-review/README.md @@ -76,10 +76,12 @@ in. `success>cancelled` is a cancellation that arrived after the answer. The ans then posts its own thumb, so a late-cancellation case proves the outcome through the steps rather than placing a reaction by hand. -Three properties every case holds to. A human's reaction is never withdrawn. Neither is +Four properties every case holds to. A human's reaction is never withdrawn. Neither is a reaction of this bot's that no step here chooses, so a `rocket` some other workflow -left survives. And a thumb that answers a verdict already on the pull request survives a -later cancellation — withdrawing it would read as never answered. +left survives. A thumb that answers a verdict already on the pull request survives a +later cancellation. And a run cancelled before it reached `Answer the request` takes +only the eyes: a thumb on the comment then belongs to an EARLIER run, whose verdict may +still stand. # The step conditions @@ -93,12 +95,16 @@ It models the runner's own rule that a condition naming none of `always`/`cancelled`/`failure`/`success` is stored as `success() && (...)`, and treats any term it does not decide as unknown rather than as false. -Two checks are stated over the file rather than over a table, so a step added later is -covered: +Two checks are stated over the file rather than over a table, so they cover a step +added later. Both walk **every job's raw steps list**, so an unnamed step is not +invisible to them, and both search the **whole step** rather than one key: - No step that can run on a cancelled job may reach `check_path` or - `verdict_produced`. The haystack is the whole step, not its `env` block: an inline - `${{ steps.drive.outputs.check_path }}` in `run:`, `with:` or `if:` reaches the same - value. -- Every `steps.` a reaction step's condition reads must be a real id on an earlier - step. Delete the id and the read is empty forever, with no error anywhere. + `verdict_produced`. An inline `${{ steps.drive.outputs.check_path }}` in `run:`, + `with:` or `if:` reaches the same value an `env:` key would. +- Every `steps.` a step reads must be a real id on an earlier step. Delete the id, + or move the reader in front of it, and the read is empty forever with no error + anywhere — and a harness that takes the value as an argument cannot notice. + +Neither check needs telling where to look. A check that has to be pointed at a step is +not stated over the file. diff --git a/test/seidroid-review/conditions.py b/test/seidroid-review/conditions.py index 8156ae6..c8555cc 100644 --- a/test/seidroid-review/conditions.py +++ b/test/seidroid-review/conditions.py @@ -28,12 +28,6 @@ import yaml -# step -> job state -> does it run? mode is 'review' with a comment id unless said. -# -# The cancelled column is the invariant. `Answer the request` reads check_path and -# verdict_produced, and a cancellation arriving after the driver finishes leaves both -# populated -- so a run of it on a cancelled job posts a thumb for a verdict that no -# step published. # step -> (job state, mode, comment id, `Post the verdict` outcome) -> does it run? # # Two invariants live here. `Answer the request` reads check_path and verdict_produced, @@ -265,6 +259,17 @@ def stored_condition(raw): return f"success() && ({expr})" +def dump(step): + """The whole step as text. An inline ${{ }} in `run:`, `with:` or `if:` reaches the + same value an `env:` key would, so every check searches all of it.""" + return yaml.safe_dump(step, default_flow_style=False) + + +def label(step, index): + """What to call a step in a finding. A step need not have a name.""" + return step.get("name") or step.get("uses") or f"step {index + 1}" + + REACTION_STEPS = ( "Acknowledge the trigger", "Answer the request", @@ -324,51 +329,57 @@ def check(label, want, got): got, ) - # A `steps..outcome` read only works when that id exists and belongs to an - # EARLIER step. Delete the id and the read is silently empty forever, which sends - # the withdrawal down its clearing path on every cancellation -- including the one - # where the verdict published and the thumb is honest. The model takes the outcome - # as an argument, so nothing above would notice. - print("== every steps. a reaction step reads exists, and runs before it") - order = [s.get("name") for s in doc["jobs"]["review"]["steps"]] - ids = { - s["id"]: i - for i, s in enumerate(doc["jobs"]["review"]["steps"]) - if s.get("id") - } - for name in EXPECTED: - if name not in steps: - continue - reader = order.index(name) - for ref in sorted(set(re.findall(r"steps\.([A-Za-z0-9_-]+)\.", str(steps[name].get("if", ""))))): - if ref not in ids: - check(f"{name} reads steps.{ref}, which is no step's id", True, False) - elif ids[ref] > reader: - check(f"{name} reads steps.{ref}, which runs later", True, False) - else: - check(f"{name} reads steps.{ref}", True, True) - - # Whatever the table above says, no step that can reach a conclusion may run on a - # cancelled job. This is the invariant, stated over the file rather than over the - # table, so a step added later is covered too. - print("== nothing that reads a conclusion runs on a cancelled run") - for name, step in steps.items(): - expr = stored_condition(step.get("if", "success()")) - verdict = truthy( - Parser( - lex(expr), Ctx("cancelled", "review", "7", lenient=True) - ).parse() - ) - # UNKNOWN counts as "can run": the check must not pass because a term went - # unmodelled. - runs = verdict is not False - haystack = yaml.safe_dump(step, default_flow_style=False) - reads = [k for k in CONCLUSION_INPUTS if k in haystack] - if runs and reads: - print(f" FAIL {name} runs on a cancelled run and reads {', '.join(reads)}") - failed += 1 - else: - passed += 1 + # Both checks below walk the RAW steps list. Keying them off a name drops an + # unnamed step, and `- uses: actions/checkout@v7` with no `name:` is the usual + # shape -- so the very step most likely to arrive later would be the one the + # invariant could not see. The guard job already carries one unnamed step. + # + # Every job, not only `review`: a reaction step could be added anywhere, and a + # check that has to be told where to look is not stated over the file. + for job_name, job in doc["jobs"].items(): + raw = job.get("steps") or [] + ids = {st["id"]: i for i, st in enumerate(raw) if st.get("id")} + + # A steps. read only works when that id exists and belongs to an EARLIER + # step. Delete the id, or move the reader in front of it, and the read is + # silently empty forever. The model takes an outcome as an argument, so nothing + # above would notice. + # + # The whole step, not its `if`: the withdrawal set reads an outcome through + # `env`, and `run:` and `with:` reach the same values. + print(f"== every steps. read in job '{job_name}' exists, and runs before it") + for i, st in enumerate(raw): + where = label(st, i) + for ref in sorted(set(re.findall(r"steps\.([A-Za-z0-9_-]+)\.", dump(st)))): + if ref not in ids: + check(f"{where} reads steps.{ref}, which is no step's id", True, False) + elif ids[ref] > i: + check(f"{where} reads steps.{ref}, which runs later", True, False) + else: + check(f"{where} reads steps.{ref}", True, True) + + # No step that can run on a cancelled job may reach a conclusion, or a cancelled + # run can state an outcome. Stated over the file rather than over the table + # above, so a step added later is covered too. + print(f"== nothing in job '{job_name}' that reads a conclusion runs on a cancelled run") + for i, st in enumerate(raw): + where = label(st, i) + expr = stored_condition(st.get("if", "success()")) + outcome = truthy( + Parser( + lex(expr), Ctx("cancelled", "review", "7", lenient=True) + ).parse() + ) + # UNKNOWN counts as "can run": the check must not pass because a term went + # unmodelled. + runs = outcome is not False + reads = [k for k in CONCLUSION_INPUTS if k in dump(st)] + check( + f"{where}" + + (f" runs on a cancelled run and reads {', '.join(reads)}" if reads else ""), + True, + not (runs and reads), + ) print(f"\nassertions: {passed} passed, {failed} failed") return 1 if failed else 0 diff --git a/test/seidroid-review/reactions.sh b/test/seidroid-review/reactions.sh index fdbdb4c..f752e33 100755 --- a/test/seidroid-review/reactions.sh +++ b/test/seidroid-review/reactions.sh @@ -40,6 +40,9 @@ HUMAN_ALL='[{"id":21,"content":"+1","user":{"login":"brandon"}}, STALE_DOWN='[{"id":31,"content":"-1","user":{"login":"github-actions[bot]"}}, {"id":21,"content":"+1","user":{"login":"brandon"}}]' STALE_UP='[{"id":32,"content":"+1","user":{"login":"github-actions[bot]"}}]' +# An EARLIER run's thumb, on a comment a re-run replays. Its verdict is on the pull +# request, so it is not this run's to take. +EARLIER_THUMB='[{"id":51,"content":"+1","user":{"login":"github-actions[bot]"}}]' # A reaction of this bot's that no step here ever chooses. Whatever put it there owns it. FOREIGN='[{"id":41,"content":"rocket","user":{"login":"github-actions[bot]"}}, {"id":21,"content":"+1","user":{"login":"brandon"}}]' @@ -66,6 +69,10 @@ run_case() { shift 8 CASE="$HERE/out-reactions/$name" rm -rf "$CASE"; mkdir -p "$CASE" + # Every per-case knob is cleared here, not at the end of the case that set it. An + # export leaks to every later case otherwise, and a case that stops exercising what + # it claims fails nothing. + unset ANSWERED_AS ACK_POST SKIP_ACK export STUB_STATE="$CASE/reactions.json"; printf '%s\n' "$seed" > "$STUB_STATE" export STUB_LIST=ok STUB_DELETE=ok STUB_POST=ok STUB_ACTOR="$BOT" export PATH="$HERE/bin-reactions:$PATH" @@ -104,6 +111,16 @@ run_case() { done < <($SELECT "$1" review 7 "$2") } run_selected "$answer_state" "$verdict_outcome" "Answer the request" + # The withdrawal reads the answer step's outcome to decide what it may take. Derived + # from whether the harness just ran that step, not passed in, so a case cannot claim + # an outcome the timeline it declared would not produce. ANSWERED_AS overrides it, for + # the arms a two-state timeline cannot reach. + if [ "$(ran 'Answer the request')" = 1 ]; then + ANSWERED="${ANSWERED_AS-success}" + else + ANSWERED="${ANSWERED_AS-skipped}" + fi + export ANSWERED run_selected "$state" "$verdict_outcome" "Withdraw the reactions on a cancelled run" echo "$rc" > "$CASE/rc" @@ -167,6 +184,38 @@ check "withdraw skipped" 0 "$(ran 'Withdraw the reactions on a cancelled run')" check "one post, no later delete" "1 1" "$(calls post) $(calls delete)" check "THUMB SURVIVES" "$BOT:+1" "$(left)" +echo "== A RE-RUN REPLAYS THE TRIGGER COMMENT ID, so an earlier run's thumb can already" +echo " be on it. A run cancelled before it answers has posted only the eyes, and that" +echo " thumb answers a verdict still on the pull request. ==" +run_case rerun-cancelled-before-answer "$EARLIER_THUMB" cancelled cancelled skipped - '' no +check "answer never ran" 0 "$(ran 'Answer the request')" +check "withdraw ran" 1 "$(ran 'Withdraw the reactions on a cancelled run')" +check "took the eyes only" 1 "$(calls delete)" +check "EARLIER THUMB SURVIVES" "$BOT:+1" "$(left)" + +echo "== the same, beside a human's ==" +run_case rerun-cancelled-human \ + '[{"id":51,"content":"+1","user":{"login":"github-actions[bot]"}}, + {"id":23,"content":"-1","user":{"login":"brandon"}}]' \ + cancelled cancelled skipped - '' no +check "left" "brandon:-1 $BOT:+1" "$(left)" + +echo "== but once this run has answered, every reaction on the comment is its own ==" +run_case rerun-answered-then-cancelled "$EARLIER_THUMB" success cancelled skipped success true yes +check "answer ran" 1 "$(ran 'Answer the request')" +check "withdraw ran" 1 "$(ran 'Withdraw the reactions on a cancelled run')" +check "left" "" "$(left)" + +echo "== a partial answer clears: it most likely took the earlier thumb already ==" +run_case rerun-answer-failed "$EARLIER_THUMB" cancelled cancelled skipped - '' no ANSWERED_AS=failure +check "left" "" "$(left)" +run_case rerun-answer-cancelled "$EARLIER_THUMB" cancelled cancelled skipped - '' no ANSWERED_AS=cancelled +check "left" "" "$(left)" + +echo "== an outcome the step cannot read clears: a thumb standing for nothing is worse ==" +run_case rerun-answer-unreported "$EARLIER_THUMB" cancelled cancelled skipped - '' no ANSWERED_AS= +check "left" "" "$(left)" + echo "== and it survives beside a human's reactions ==" run_case cancelled-after-publish-human \ '[{"id":21,"content":"-1","user":{"login":"brandon"}}]' \ @@ -183,11 +232,13 @@ check "left" "brandon:+1 brandon:-1 brandon:eyes" "$(left)" run_case cancelled-human "$HUMAN_ALL" cancelled cancelled skipped success true yes check "left" "brandon:+1 brandon:-1 brandon:eyes" "$(left)" -echo "== a stale thumb from an earlier attempt goes, a human's stays ==" +echo "== a run that answers replaces the stale thumb; a human's stays ==" run_case stale-thumb "$STALE_DOWN" success success success success true yes check "left" "brandon:+1 $BOT:+1" "$(left)" + +echo "== a cancelled run that never answered leaves it: the earlier verdict may stand ==" run_case stale-thumb-cancelled "$STALE_DOWN" cancelled cancelled skipped success true yes -check "left" "brandon:+1" "$(left)" +check "left" "brandon:+1 $BOT:-1" "$(left)" run_case stale-up-noverdict "$STALE_UP" success success skipped failure false yes check "left" "" "$(left)" @@ -222,7 +273,6 @@ check "rc" 0 "$(cat "$CASE/rc")" check "ack warned" 1 "$(grep -c '::warning::could not react to comment' "$CASE/ack.out")" check "nothing to clear" 0 "$(calls delete)" check "left" "$BOT:+1" "$(left)" -unset ACK_POST echo "== a close reacts nowhere, so nothing is left to clear ==" CASE="$HERE/out-reactions/close-mode"; rm -rf "$CASE"; mkdir -p "$CASE" From a003d606f62e212d27b52a2df7abaf152ad59522 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 7 Sep 2026 13:34:32 -0700 Subject: [PATCH 6/7] fix(seidroid-review): gate the withdrawal on whether the verdict landed, not on an exit code `steps.verdict.outcome` cannot carry the fact the gate needs. `Post the verdict` tolerates a refused comment POST and ends on a call whose failure it swallows, so it exits 0 and its outcome reads `success` whether the verdict landed or not. A later cancellation then skipped the withdrawal and left the thumb `Answer the request` posted, standing for a review nobody can see. That step already tracks the POST's result in a shell variable. It now writes it as a `posted` output, and the withdrawal reads `steps.verdict.outputs.posted != 'true'`. One boolean about another step, written from the POST's own result, with no conclusion in it -- so the structural property holds: the withdrawal step still has no POST. `conditions.py` models `steps.verdict.outcome` beside `posted`, derived rather than passed: `success` whenever that step reported at all, which is what the runner sees. A condition that goes back to reading the outcome therefore fails an assertion instead of crashing the model. New case `cancelled-publish-failed`: the answer step posts its thumb, the POST of the verdict is refused, and the withdrawal takes the thumb. Reverting the gate to the outcome fails it, and fails the matching condition row. Reported by Cursor Bugbot on 1ba2010. PLT-1166 Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/seidroid-review.yml | 34 ++++++---- test/seidroid-review/README.md | 6 ++ test/seidroid-review/conditions.py | 59 ++++++++++------- test/seidroid-review/reactions.sh | 94 +++++++++++++++------------ 4 files changed, 114 insertions(+), 79 deletions(-) diff --git a/.github/workflows/seidroid-review.yml b/.github/workflows/seidroid-review.yml index c2685d7..860e6a6 100644 --- a/.github/workflows/seidroid-review.yml +++ b/.github/workflows/seidroid-review.yml @@ -2524,8 +2524,8 @@ jobs: fi - name: Post the verdict - # id: the withdrawal step at the end of this job reads this step's outcome to - # decide whether the trigger comment already carries an honest answer. + # id: the withdrawal step at the end of this job reads this step's `posted` + # output to decide whether the trigger comment already carries an honest answer. id: verdict # Post only when a real verdict was produced, and even when the drive # step above exited non-zero -- `!cancelled()` runs on any outcome @@ -2731,6 +2731,13 @@ jobs: if gh api -X POST "repos/$REPO/issues/$PR/comments" -f body="$body" >/dev/null; then posted=true fi + # Whether the verdict is ON the pull request, for the withdrawal step at the + # end of this job: a thumb on the trigger comment answers a verdict only when + # the verdict is there to read. This step's exit status cannot carry that. The + # failure path below ends on a call whose failure it swallows, so the step + # exits 0 whether the comment landed or not, and its outcome reads success + # either way. + echo "posted=$posted" >> "$GITHUB_OUTPUT" if [ "$posted" = true ]; then echo "posted the verdict on $REPO#$PR" # An earlier run may have left a notice saying this review did not complete. @@ -3205,15 +3212,20 @@ jobs: # review with no reaction on the request that asked for it, which reads as never # answered. That is this step's own defect, one window later. # - # The posting step's OUTCOME is what separates the two, and it is four words - # about another step: success, failure, cancelled or skipped. None of them names - # a conclusion. It says whether an answer already stands, never which answer it - # would be, so reading it cannot give this step an outcome to state. The same - # holds for the answer step's outcome, which the withdrawal set below reads. + # Its `posted` OUTPUT separates the two, and its outcome does not. That step + # tolerates a failed comment POST and ends on a call whose failure it swallows, + # so it exits 0 and reads success whether the verdict landed or not. A thumb kept + # on that reading would stand for a review nobody can see. The output is written + # from the POST's own result. + # + # One boolean about another step, with no conclusion in it. It says whether an + # answer already stands, never which answer it would be, so reading it cannot + # give this step an outcome to state. The same holds for the answer step's + # outcome, which the withdrawal set below reads. # - # Anything but success withdraws. An outcome this step cannot read therefore - # clears, rather than leaving a thumb that stands for a verdict which may not be - # on the pull request. + # Anything but a posted verdict withdraws. A value this step cannot read + # therefore clears, rather than leaving a thumb that stands for a verdict which + # may not be on the pull request. # # WHAT IT PROTECTS, AND WHERE THAT STOPS. A thumb survives when the verdict it # answers is on the pull request AND this run is the run that posted it, or when @@ -3235,7 +3247,7 @@ jobs: # cannot be handed to a shell and this decision cannot move inside one script. if: ${{ inputs.mode == 'review' && cancelled() && needs.guard.outputs.comment_id != '' - && steps.verdict.outcome != 'success' }} + && steps.verdict.outputs.posted != 'true' }} continue-on-error: true shell: bash env: diff --git a/test/seidroid-review/README.md b/test/seidroid-review/README.md index 5130d6f..5cfb097 100644 --- a/test/seidroid-review/README.md +++ b/test/seidroid-review/README.md @@ -76,6 +76,12 @@ in. `success>cancelled` is a cancellation that arrived after the answer. The ans then posts its own thumb, so a late-cancellation case proves the outcome through the steps rather than placing a reaction by hand. +A case's `posted` column is `Post the verdict`'s own output: `true` when its comment +landed, `false` when the POST was refused, unreported when that step never ran. Not its +outcome. That step tolerates a refused POST and exits 0 either way, so its outcome reads +`success` on a verdict that never landed, and a thumb kept on that reading would stand +for a review nobody can see. + Four properties every case holds to. A human's reaction is never withdrawn. Neither is a reaction of this bot's that no step here chooses, so a `rocket` some other workflow left survives. A thumb that answers a verdict already on the pull request survives a diff --git a/test/seidroid-review/conditions.py b/test/seidroid-review/conditions.py index c8555cc..004e56c 100644 --- a/test/seidroid-review/conditions.py +++ b/test/seidroid-review/conditions.py @@ -17,7 +17,7 @@ cancelled run without saying so. conditions.py - conditions.py --select + conditions.py --select --select names the reaction steps GitHub runs in that state, in job order. reactions.sh uses it, so no case there hardcodes which step a state selects and the @@ -28,14 +28,15 @@ import yaml -# step -> (job state, mode, comment id, `Post the verdict` outcome) -> does it run? +# step -> (job state, mode, comment id, `Post the verdict` posted?) -> does it run? # # Two invariants live here. `Answer the request` reads check_path and verdict_produced, # and a cancellation arriving after the driver finishes leaves both populated -- so a # run of it on a cancelled job posts a thumb for a verdict no step published. And the -# withdrawal must not strip a thumb that answers a verdict which DID publish: a -# cancellation can arrive after `Post the verdict` upserted the comment, and the -# trigger comment then carries an honest answer. +# withdrawal must not strip a thumb that answers a verdict which DID land: a +# cancellation can arrive after `Post the verdict` posted the comment, and the trigger +# comment then carries an honest answer. That step's `posted` output is the fact, not +# its outcome, which reads success even when the POST was refused. EXPECTED = { "Acknowledge the trigger": { ("success", "review", "7", ""): True, @@ -48,26 +49,24 @@ ("success", "review", "7", ""): True, ("failure", "review", "7", ""): True, ("cancelled", "review", "7", ""): False, - ("cancelled", "review", "7", "success"): False, + ("cancelled", "review", "7", "true"): False, ("success", "review", "", ""): False, ("success", "close", "7", ""): False, ("cancelled", "close", "7", ""): False, }, "Withdraw the reactions on a cancelled run": { # Nothing to withdraw on a run that was not cancelled. - ("success", "review", "7", "success"): False, - ("failure", "review", "7", "failure"): False, - # Cancelled before the verdict published: withdraw. - ("cancelled", "review", "7", "skipped"): True, - ("cancelled", "review", "7", "cancelled"): True, - ("cancelled", "review", "7", "failure"): True, - # An outcome this step cannot read clears rather than leaving a thumb. + ("success", "review", "7", "true"): False, + ("failure", "review", "7", "false"): False, + # Cancelled before the verdict landed: withdraw. + ("cancelled", "review", "7", "false"): True, + # A value this step cannot read clears rather than leaving a thumb. ("cancelled", "review", "7", ""): True, # THE CASE FOR THE LATE WINDOW. The verdict is on the pull request and the # thumb answers it, so the thumb stays. - ("cancelled", "review", "7", "success"): False, - ("cancelled", "review", "", "skipped"): False, - ("cancelled", "close", "7", "skipped"): False, + ("cancelled", "review", "7", "true"): False, + ("cancelled", "review", "", "false"): False, + ("cancelled", "close", "7", "false"): False, }, } @@ -116,15 +115,25 @@ def __repr__(self): class Ctx: - def __init__(self, state, mode, comment_id, verdict_outcome="", lenient=False): + def __init__(self, state, mode, comment_id, posted="", lenient=False): self.state = state self.lenient = lenient self.values = { "inputs.mode": mode, "needs.guard.outputs.comment_id": comment_id, - # One of success, failure, cancelled, skipped, or empty for a step that - # never reported. Four words about another step; none of them a conclusion. - "steps.verdict.outcome": verdict_outcome, + # `true` when the verdict comment landed on the pull request, `false` when + # the POST was refused, empty when that step never reported. One boolean + # about another step, with no conclusion in it. + "steps.verdict.outputs.posted": posted, + # Modelled BESIDE it, and derived rather than passed, so a condition that + # goes back to reading the outcome fails an assertion instead of crashing + # this model. `Post the verdict` tolerates a refused POST and ends on a + # call whose failure it swallows, so it exits 0 and reads success whenever + # it reported at all -- including on a verdict that never landed. That is + # why the gate cannot use it. + "steps.verdict.outcome": ( + "success" if posted in ("true", "false") else "skipped" + ), } def func(self, name): @@ -284,13 +293,13 @@ def load(path): def select(argv): """Print the reaction steps that run, one per line, in job order.""" - workflow, state, mode, cid, verdict_outcome = argv + workflow, state, mode, cid, posted = argv steps = {s["name"]: s for s in load(workflow)["jobs"]["review"]["steps"] if "name" in s} for name in REACTION_STEPS: if name not in steps: continue expr = stored_condition(steps[name].get("if", "success()")) - ctx = Ctx(state, mode, cid, "" if verdict_outcome == "-" else verdict_outcome) + ctx = Ctx(state, mode, cid, "" if posted == "-" else posted) if truthy(Parser(lex(expr), ctx).parse()) is True: print(name) return 0 @@ -320,11 +329,11 @@ def check(label, want, got): continue expr = stored_condition(steps[name].get("if", "success()")) print(f"== {name}\n {expr}") - for (state, mode, cid, vo), want in cases.items(): - got = truthy(Parser(lex(expr), Ctx(state, mode, cid, vo)).parse()) + for (state, mode, cid, posted), want in cases.items(): + got = truthy(Parser(lex(expr), Ctx(state, mode, cid, posted)).parse()) check( f"{name} / {state} / {mode} / id={cid or 'empty'}" - f" / verdict={vo or 'unreported'}", + f" / posted={posted or 'unreported'}", want, got, ) diff --git a/test/seidroid-review/reactions.sh b/test/seidroid-review/reactions.sh index f752e33..42b531f 100755 --- a/test/seidroid-review/reactions.sh +++ b/test/seidroid-review/reactions.sh @@ -3,8 +3,8 @@ # what each case leaves on the trigger comment. # # No case names the step it runs. `conditions.py --select` names it, from the job state -# and from `Post the verdict`'s outcome, so the shell layer and the condition layer -# cannot drift and a case cannot quietly stop exercising the step it claims to. +# and from whether `Post the verdict` landed its comment, so the shell layer and the +# condition layer cannot drift, and a case cannot stop exercising the step it claims to. set -uo pipefail HERE="$(cd "$(dirname "$0")" && pwd)" REPOROOT="$(cd "$HERE/../.." && pwd)" @@ -55,16 +55,20 @@ left() { jq -r '[.[] | "\(.user.login):\(.content)"] | sort | join(" ")' < "$STU calls() { grep -c "^CALL $1" "$CASE/calls.log" || true; } ran() { grep -c "^$1\$" "$CASE/ran.txt" || true; } -# run_case \ +# run_case \ # [K=V...] # # Two states, because a cancellation has a moment. `answer state` is the job state when # the runner reached `Answer the request`; `final state` is the state when it reached the -# withdrawal step at the end. A cancellation arriving after the verdict published gives +# withdrawal step at the end. A cancellation arriving after the verdict landed gives # success then cancelled, so the answer step posts its thumb and the fixture does not # have to place one. +# +# `posted` is `Post the verdict`'s own output: true when its comment landed, false when +# the POST was refused, `-` when that step never reported. Not its outcome, which reads +# success even on a refused POST. run_case() { - local name="$1" seed="$2" answer_state="$3" state="$4" verdict_outcome="$5" + local name="$1" seed="$2" answer_state="$3" state="$4" posted="$5" local conclusion="$6" produced="$7" have_check="$8" shift 8 CASE="$HERE/out-reactions/$name" @@ -110,7 +114,7 @@ run_case() { echo "$step" >> "$CASE/ran.txt" done < <($SELECT "$1" review 7 "$2") } - run_selected "$answer_state" "$verdict_outcome" "Answer the request" + run_selected "$answer_state" "$posted" "Answer the request" # The withdrawal reads the answer step's outcome to decide what it may take. Derived # from whether the harness just ran that step, not passed in, so a case cannot claim # an outcome the timeline it declared would not produce. ANSWERED_AS overrides it, for @@ -121,55 +125,59 @@ run_case() { ANSWERED="${ANSWERED_AS-skipped}" fi export ANSWERED - run_selected "$state" "$verdict_outcome" "Withdraw the reactions on a cancelled run" + run_selected "$state" "$posted" "Withdraw the reactions on a cancelled run" echo "$rc" > "$CASE/rc" - rows+=("$(printf '%-29s %-19s verdict=%-10s ran=%-9s list=%s del=%s post=%s left=%s' \ - "$name" "$answer_state>$state" "${verdict_outcome/-/unreported}" \ + rows+=("$(printf '%-29s %-19s posted=%-10s ran=%-9s list=%s del=%s post=%s left=%s' \ + "$name" "$answer_state>$state" "${posted/-/unreported}" \ "$(sed -n 's/^Answer the request$/answer/p;s/^Withdraw.*/withdraw/p' \ "$CASE/ran.txt" | paste -sd+ - || true)" \ "$(calls list)" "$(calls delete)" "$(calls post)" "$(left)")") } echo "== a green review thumbs the request up ==" -run_case success "$NONE" success success success success true yes +run_case success "$NONE" success success true success true yes check "answer ran" 1 "$(ran 'Answer the request')" check "left" "$BOT:+1" "$(left)" echo "== a blocking review thumbs it down ==" -run_case failure "$NONE" success success success failure true yes +run_case failure "$NONE" success success true failure true yes check "left" "$BOT:-1" "$(left)" echo "== a run that reached no verdict clears and says nothing ==" -run_case no-verdict "$NONE" success success skipped failure false yes +run_case no-verdict "$NONE" success success - failure false yes check "left" "" "$(left)" check "no post" 0 "$(calls post)" echo "== neutral earns no reaction ==" -run_case neutral "$NONE" success success skipped neutral true yes +run_case neutral "$NONE" success success - neutral true yes check "left" "" "$(left)" echo "== a cancelled run with the verdict outputs POPULATED still posts nothing." echo " A cancellation after the driver finishes leaves a real conclusion on disk. ==" -run_case cancelled-after-drive "$NONE" cancelled cancelled skipped success true yes +run_case cancelled-after-drive "$NONE" cancelled cancelled - success true yes check "withdraw ran" 1 "$(ran 'Withdraw the reactions on a cancelled run')" check "answer skipped" 0 "$(ran 'Answer the request')" check "left" "" "$(left)" check "no post" 0 "$(calls post)" echo "== a cancelled run before the driver finishes ==" -run_case cancelled-early "$NONE" cancelled cancelled skipped - '' no +run_case cancelled-early "$NONE" cancelled cancelled - - '' no check "left" "" "$(left)" check "no post" 0 "$(calls post)" echo "== cancelled while the verdict was posting: the thumb goes, it may not have landed ==" -run_case cancelled-mid-publish "$NONE" success cancelled cancelled success true yes +run_case cancelled-mid-publish "$NONE" success cancelled - success true yes check "withdraw ran" 1 "$(ran 'Withdraw the reactions on a cancelled run')" check "left" "" "$(left)" -echo "== the verdict failed to post: the thumb goes, it would stand for nothing ==" -run_case cancelled-publish-failed "$NONE" success cancelled failure success true yes -check "left" "" "$(left)" +echo "== THE VERDICT COMMENT WAS REFUSED. `Post the verdict` tolerates that and exits 0," +echo " so its OUTCOME reads success while nothing landed. The thumb has to go: reading" +echo " the outcome here would leave it standing for a review nobody can see. ==" +run_case cancelled-publish-failed "$NONE" success cancelled false success true yes +check "answer posted a thumb" 1 "$(calls post)" +check "withdraw ran" 1 "$(ran 'Withdraw the reactions on a cancelled run')" +check "THUMB GOES" "" "$(left)" echo "== an outcome this step cannot read clears rather than leaving a thumb ==" run_case cancelled-unreported "$NONE" success cancelled - success true yes @@ -178,7 +186,7 @@ check "left" "" "$(left)" echo "== CANCELLED AFTER THE VERDICT PUBLISHED. The thumb answers a review that is on" echo " the pull request, so it survives: withdrawing it would read as never answered. ==" -run_case cancelled-after-publish "$NONE" success cancelled success success true yes +run_case cancelled-after-publish "$NONE" success cancelled true success true yes check "answer ran" 1 "$(ran 'Answer the request')" check "withdraw skipped" 0 "$(ran 'Withdraw the reactions on a cancelled run')" check "one post, no later delete" "1 1" "$(calls post) $(calls delete)" @@ -187,7 +195,7 @@ check "THUMB SURVIVES" "$BOT:+1" "$(left)" echo "== A RE-RUN REPLAYS THE TRIGGER COMMENT ID, so an earlier run's thumb can already" echo " be on it. A run cancelled before it answers has posted only the eyes, and that" echo " thumb answers a verdict still on the pull request. ==" -run_case rerun-cancelled-before-answer "$EARLIER_THUMB" cancelled cancelled skipped - '' no +run_case rerun-cancelled-before-answer "$EARLIER_THUMB" cancelled cancelled - - '' no check "answer never ran" 0 "$(ran 'Answer the request')" check "withdraw ran" 1 "$(ran 'Withdraw the reactions on a cancelled run')" check "took the eyes only" 1 "$(calls delete)" @@ -197,78 +205,78 @@ echo "== the same, beside a human's ==" run_case rerun-cancelled-human \ '[{"id":51,"content":"+1","user":{"login":"github-actions[bot]"}}, {"id":23,"content":"-1","user":{"login":"brandon"}}]' \ - cancelled cancelled skipped - '' no + cancelled cancelled - - '' no check "left" "brandon:-1 $BOT:+1" "$(left)" echo "== but once this run has answered, every reaction on the comment is its own ==" -run_case rerun-answered-then-cancelled "$EARLIER_THUMB" success cancelled skipped success true yes +run_case rerun-answered-then-cancelled "$EARLIER_THUMB" success cancelled - success true yes check "answer ran" 1 "$(ran 'Answer the request')" check "withdraw ran" 1 "$(ran 'Withdraw the reactions on a cancelled run')" check "left" "" "$(left)" echo "== a partial answer clears: it most likely took the earlier thumb already ==" -run_case rerun-answer-failed "$EARLIER_THUMB" cancelled cancelled skipped - '' no ANSWERED_AS=failure +run_case rerun-answer-failed "$EARLIER_THUMB" cancelled cancelled - - '' no ANSWERED_AS=failure check "left" "" "$(left)" -run_case rerun-answer-cancelled "$EARLIER_THUMB" cancelled cancelled skipped - '' no ANSWERED_AS=cancelled +run_case rerun-answer-cancelled "$EARLIER_THUMB" cancelled cancelled - - '' no ANSWERED_AS=cancelled check "left" "" "$(left)" echo "== an outcome the step cannot read clears: a thumb standing for nothing is worse ==" -run_case rerun-answer-unreported "$EARLIER_THUMB" cancelled cancelled skipped - '' no ANSWERED_AS= +run_case rerun-answer-unreported "$EARLIER_THUMB" cancelled cancelled - - '' no ANSWERED_AS= check "left" "" "$(left)" echo "== and it survives beside a human's reactions ==" run_case cancelled-after-publish-human \ '[{"id":21,"content":"-1","user":{"login":"brandon"}}]' \ - success cancelled success success true yes + success cancelled true success true yes check "left" "brandon:-1 $BOT:+1" "$(left)" echo "== a human's reaction survives every path ==" -run_case success-human "$HUMAN_ALL" success success success success true yes +run_case success-human "$HUMAN_ALL" success success true success true yes check "left" "brandon:+1 brandon:-1 brandon:eyes $BOT:+1" "$(left)" -run_case failure-human "$HUMAN_ALL" success success success failure true yes +run_case failure-human "$HUMAN_ALL" success success true failure true yes check "left" "brandon:+1 brandon:-1 brandon:eyes $BOT:-1" "$(left)" -run_case noverdict-human "$HUMAN_ALL" success success skipped failure false yes +run_case noverdict-human "$HUMAN_ALL" success success - failure false yes check "left" "brandon:+1 brandon:-1 brandon:eyes" "$(left)" -run_case cancelled-human "$HUMAN_ALL" cancelled cancelled skipped success true yes +run_case cancelled-human "$HUMAN_ALL" cancelled cancelled - success true yes check "left" "brandon:+1 brandon:-1 brandon:eyes" "$(left)" echo "== a run that answers replaces the stale thumb; a human's stays ==" -run_case stale-thumb "$STALE_DOWN" success success success success true yes +run_case stale-thumb "$STALE_DOWN" success success true success true yes check "left" "brandon:+1 $BOT:+1" "$(left)" echo "== a cancelled run that never answered leaves it: the earlier verdict may stand ==" -run_case stale-thumb-cancelled "$STALE_DOWN" cancelled cancelled skipped success true yes +run_case stale-thumb-cancelled "$STALE_DOWN" cancelled cancelled - success true yes check "left" "brandon:+1 $BOT:-1" "$(left)" -run_case stale-up-noverdict "$STALE_UP" success success skipped failure false yes +run_case stale-up-noverdict "$STALE_UP" success success - failure false yes check "left" "" "$(left)" echo "== a reaction no step here chooses is not this job's to withdraw ==" -run_case foreign "$FOREIGN" success success success success true yes +run_case foreign "$FOREIGN" success success true success true yes check "left" "brandon:+1 $BOT:+1 $BOT:rocket" "$(left)" -run_case foreign-cancelled "$FOREIGN" cancelled cancelled skipped success true yes +run_case foreign-cancelled "$FOREIGN" cancelled cancelled - success true yes check "left" "brandon:+1 $BOT:rocket" "$(left)" echo "== a refused call warns and never fails the step ==" run_case list-refused '[{"id":21,"content":"+1","user":{"login":"brandon"}}]' \ - success success success success true yes STUB_LIST=FAIL + success success true success true yes STUB_LIST=FAIL check "rc" 0 "$(cat "$CASE/rc")" check "warned" 1 "$(grep -c '::warning::could not read the reactions' "$CASE/step.out")" check "thumb still" 1 "$(calls post)" -run_case delete-refused "$NONE" success success success success true yes STUB_DELETE=FAIL +run_case delete-refused "$NONE" success success true success true yes STUB_DELETE=FAIL check "rc" 0 "$(cat "$CASE/rc")" check "eyes stay" "$BOT:+1 $BOT:eyes" "$(left)" -run_case post-refused "$NONE" success success success success true yes STUB_POST=FAIL +run_case post-refused "$NONE" success success true success true yes STUB_POST=FAIL check "rc" 0 "$(cat "$CASE/rc")" check "left" "" "$(left)" -run_case list-refused-cancelled "$NONE" cancelled cancelled skipped - '' no STUB_LIST=FAIL +run_case list-refused-cancelled "$NONE" cancelled cancelled - - '' no STUB_LIST=FAIL check "rc" 0 "$(cat "$CASE/rc")" check "warned" 1 "$(grep -c '::warning::could not read the reactions' "$CASE/step.out")" -run_case delete-refused-cancelled "$NONE" cancelled cancelled skipped - '' no STUB_DELETE=FAIL +run_case delete-refused-cancelled "$NONE" cancelled cancelled - - '' no STUB_DELETE=FAIL check "rc" 0 "$(cat "$CASE/rc")" check "eyes stay" "$BOT:eyes" "$(left)" echo "== the acknowledgement itself refused: no eyes to clear, the answer still lands ==" -run_case ack-refused "$NONE" success success success success true yes ACK_POST=FAIL +run_case ack-refused "$NONE" success success true success true yes ACK_POST=FAIL check "rc" 0 "$(cat "$CASE/rc")" check "ack warned" 1 "$(grep -c '::warning::could not react to comment' "$CASE/ack.out")" check "nothing to clear" 0 "$(calls delete)" @@ -277,7 +285,7 @@ check "left" "$BOT:+1" "$(left)" echo "== a close reacts nowhere, so nothing is left to clear ==" 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 skipped | paste -sd, -)" +check "cancelled close selects no step" "" "$($SELECT cancelled close 7 false | paste -sd, -)" echo "== no step made a call the stub does not serve ==" check "unstubbed calls" 0 \ From 0d925ab9fc9d720938a0e2b8af98c80a5746239f Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 7 Sep 2026 13:46:37 -0700 Subject: [PATCH 7/7] test(seidroid-review): assert the withdrawal is the last step of the job The withdrawal being last is load-bearing, and nothing checked it. The runner evaluates a condition when it reaches the step, so any step placed after the withdrawal is a step during which a cancellation leaves the eyes standing: the withdrawal was already evaluated and skipped by then. `conditions.py` now checks the position rather than any one ordering. Moving the step ahead of `Post the verdict`, ahead of the resolve step, ahead of the no-verdict report, or appending a step after it all fail. The id-ordering check caught only the first of those, because `Post the verdict` still ran earlier in the other three. The README said "Two checks" where there are three, and described the withdrawal gate as `Post the verdict` having "completed" rather than having landed its comment. Both now match what the file does. PLT-1166 Co-Authored-By: Claude Opus 5 (1M context) --- test/seidroid-review/README.md | 16 ++++++++++------ test/seidroid-review/conditions.py | 18 ++++++++++++++++-- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/test/seidroid-review/README.md b/test/seidroid-review/README.md index 5cfb097..734df8c 100644 --- a/test/seidroid-review/README.md +++ b/test/seidroid-review/README.md @@ -95,15 +95,15 @@ still stand. reaction step runs in which job state, and that is where the cancellation behaviour lives. `Answer the request` reads a conclusion and may thumb the request, so it must skip a cancelled run. The withdrawal step must take a cancelled run, unless -`Post the verdict` already completed. +`Post the verdict` landed its comment. It models the runner's own rule that a condition naming none of `always`/`cancelled`/`failure`/`success` is stored as `success() && (...)`, and treats any term it does not decide as unknown rather than as false. -Two checks are stated over the file rather than over a table, so they cover a step -added later. Both walk **every job's raw steps list**, so an unnamed step is not -invisible to them, and both search the **whole step** rather than one key: +Three checks are stated over the file rather than over a table, so they cover a step +added later. The two that walk steps walk **every job's raw steps list**, so an unnamed +step is not invisible to them, and both search the **whole step** rather than one key: - No step that can run on a cancelled job may reach `check_path` or `verdict_produced`. An inline `${{ steps.drive.outputs.check_path }}` in `run:`, @@ -111,6 +111,10 @@ invisible to them, and both search the **whole step** rather than one key: - Every `steps.` a step reads must be a real id on an earlier step. Delete the id, or move the reader in front of it, and the read is empty forever with no error anywhere — and a harness that takes the value as an argument cannot notice. +- The withdrawal is the **last** step of the review job. The runner evaluates a + condition when it reaches the step, so any step after the withdrawal is a step during + which a cancellation leaves the eyes standing. Checking the position covers a step + appended later; mutating one ordering would not. -Neither check needs telling where to look. A check that has to be pointed at a step is -not stated over the file. +No check needs telling where to look. A check that has to be pointed at a step is not +stated over the file. diff --git a/test/seidroid-review/conditions.py b/test/seidroid-review/conditions.py index 004e56c..9688670 100644 --- a/test/seidroid-review/conditions.py +++ b/test/seidroid-review/conditions.py @@ -28,6 +28,10 @@ import yaml +# One name for the step, so the case table, the selector and the position check below +# cannot disagree about which step they mean. +WITHDRAW = "Withdraw the reactions on a cancelled run" + # step -> (job state, mode, comment id, `Post the verdict` posted?) -> does it run? # # Two invariants live here. `Answer the request` reads check_path and verdict_produced, @@ -54,7 +58,7 @@ ("success", "close", "7", ""): False, ("cancelled", "close", "7", ""): False, }, - "Withdraw the reactions on a cancelled run": { + WITHDRAW: { # Nothing to withdraw on a run that was not cancelled. ("success", "review", "7", "true"): False, ("failure", "review", "7", "false"): False, @@ -282,7 +286,7 @@ def label(step, index): REACTION_STEPS = ( "Acknowledge the trigger", "Answer the request", - "Withdraw the reactions on a cancelled run", + WITHDRAW, ) @@ -338,6 +342,16 @@ def check(label, want, got): got, ) + # The withdrawal has to be the LAST step of the review job, and that is not a + # tidiness preference. The runner evaluates a condition when it reaches the step, so + # any step placed after this one is a step during which a cancellation leaves the + # eyes standing: the withdrawal was already evaluated and skipped by then. Checking + # the position rather than mutating one ordering covers a step appended later. + print("== the withdrawal is the last step of the review job") + review_steps = doc["jobs"]["review"]["steps"] + last = label(review_steps[-1], len(review_steps) - 1) + check(f"last step is the withdrawal, not {last!r}", WITHDRAW, last) + # Both checks below walk the RAW steps list. Keying them off a name drops an # unnamed step, and `- uses: actions/checkout@v7` with no `name:` is the usual # shape -- so the very step most likely to arrive later would be the one the