diff --git a/.github/workflows/seidroid-review.yml b/.github/workflows/seidroid-review.yml index f93d3d2..860e6a6 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 @@ -974,7 +977,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 @@ -1023,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" @@ -2386,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. @@ -2398,6 +2409,15 @@ jobs: # The condition names three facts: a review turn, not cancelled, and a comment # to answer. A close produces no verdict, so a request to tear a session down # earns no answer. + # + # !cancelled() is what keeps a thumb honest, and it is not interchangeable with + # always() here. A cancellation arriving after `drive` finishes leaves + # check_path and verdict_produced populated, so this step would read a real + # conclusion and thumb the request -- while `Post the verdict` below skips and + # the verdict never reaches the pull request. A thumb reads as an answer, so + # that is worse than no reaction at all. The last step of this job withdraws + # the eyes a cancelled run leaves, and it cannot reach a conclusion to post. + # # No verdict_produced gate: the acknowledgement waits on this step, and a re-run # replays the trigger comment id, so a run reaching no verdict still has to clear # the eyes, and a thumb an earlier attempt left there. @@ -2504,6 +2524,9 @@ jobs: fi - name: Post the verdict + # id: the withdrawal step at the end of this job reads this step's `posted` + # output to decide whether the trigger comment already carries an honest answer. + id: verdict # Post only when a real verdict was produced, and even when the drive # step above exited non-zero -- `!cancelled()` runs on any outcome # except the job itself being cancelled (e.g. superseded by a newer @@ -2708,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. @@ -3157,3 +3187,124 @@ 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. + # + # THIS STEP HAS NO POST. It withdraws, and there is no code path in it that adds + # a reaction, so nothing it is told can make it state an outcome. That is what + # makes the withdrawal a step of its own rather than always() on `Answer the + # request`: that step chooses a reaction from a conclusion, and a cancellation + # arriving after the driver finishes leaves check_path and verdict_produced + # populated -- so it would read a real conclusion and thumb a review that + # published nothing. + # + # `Post the verdict` decides whether there is anything to withdraw, and the + # driver does not. A cancellation can arrive once the verdict is already on the + # pull request -- during thread resolution, say -- and the thumb `Answer the + # request` posted answers it correctly. Withdrawing it there leaves a published + # review with no reaction on the request that asked for it, which reads as never + # answered. That is this step's own defect, one window later. + # + # Its `posted` OUTPUT separates the two, and its outcome does not. That step + # tolerates a failed comment POST and ends on a call whose failure it swallows, + # so it exits 0 and reads success whether the verdict landed or not. A thumb kept + # on that reading would stand for a review nobody can see. The output is written + # from the POST's own result. + # + # One boolean about another step, with no conclusion in it. It says whether an + # answer already stands, never which answer it would be, so reading it cannot + # give this step an outcome to state. The same holds for the answer step's + # outcome, which the withdrawal set below reads. + # + # Anything but a posted verdict withdraws. A value this step cannot read + # therefore clears, rather than leaving a thumb that stands for a verdict which + # may not be on the pull request. + # + # WHAT IT PROTECTS, AND WHERE THAT STOPS. A thumb survives when the verdict it + # answers is on the pull request AND this run is the run that posted it, or when + # this run never reached the answer step at all. One case is left over: this run + # answers, which withdraws an earlier run's thumb and posts its own, and is then + # cancelled before publishing. The comment ends bare while the earlier run's + # verdict still stands. The earlier thumb is already gone by then -- the answer + # step took it -- so nothing here can put it back, and knowing it happened would + # take a read of the pull request this step deliberately does not make. A + # cancellation lands during the driver far more often than in that window. + # + # LAST in the job, deliberately. The runner evaluates a step's condition when it + # reaches the step, so a clear placed earlier is already skipped once a + # cancellation lands on a later one -- and it would leave both the eyes and the + # thumb the step above had just posted. + # + # cancelled() is only readable in a step or job `if`. It is not available in + # `env` or `run`, and a workflow carrying it there is rejected, so the job status + # cannot be handed to a shell and this decision cannot move inside one script. + if: ${{ inputs.mode == 'review' && cancelled() + && needs.guard.outputs.comment_id != '' + && steps.verdict.outputs.posted != 'true' }} + continue-on-error: true + shell: bash + env: + GH_TOKEN: ${{ github.token }} + TRIGGER_REPO: ${{ github.repository }} + TRIGGER_ID: ${{ needs.guard.outputs.comment_id }} + # Whether this run ever reached the step that answers. See the withdrawal set + # below. Four words about another step; none of them a conclusion. + ANSWERED: ${{ steps.answer.outcome }} + run: | + set -euo pipefail + # Scoped to the reacting identity, for the reason `Answer the request` states: + # a human who thumbed the request is voicing an opinion, and a cancellation is + # no licence to delete it. GITHUB_TOKEN reacts as github-actions[bot], and an + # installation token cannot ask the API which login it carries, so the login is + # named here. + me="github-actions[bot]" + # WHAT THIS RUN MAY TAKE, and it is not always all three. + # + # A re-run replays the trigger comment id, so the comment can already carry a + # thumb from an earlier run whose verdict IS on the pull request. `Answer the + # request` withdraws that thumb and posts this run's own, so once it has run, + # every reaction on the comment belongs to this run and this run published + # nothing -- take all three. + # + # A run cancelled before it reached that step has posted only the eyes. A thumb + # there answers an earlier run, and taking it leaves that run's published + # verdict with no reaction on the request, which reads as never answered. + # + # skipped is the only value that means "never touched the comment". failure and + # cancelled both mean the step ran partway and most likely cleared the earlier + # thumb already, so they clear, and so does a value this step cannot read: a + # thumb this run posted for a verdict nobody published is the worse of the two + # wrongs. + case "${ANSWERED:-}" in + skipped) takeable=" eyes " ;; + *) takeable=" +1 -1 eyes " ;; + esac + # Listed into a variable and read from it rather than through a pipe, so + # nothing this block reports can be read back as a reaction id. Paginated, + # because a busy comment carries more reactions than one page holds. + if ! mine="$(gh api "repos/$TRIGGER_REPO/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 what this run may take. A reaction of this bot's outside that set + # belongs to whatever put it there. + while read -r content rid; do + [ -n "$rid" ] || continue + case "$takeable" in *" $content "*) ;; *) continue ;; esac + if gh api -X DELETE \ + "repos/$TRIGGER_REPO/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..734df8c 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,63 @@ 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. + +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. + +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 +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 + +`conditions.py` covers what a shell harness cannot see. A step condition decides which +reaction step runs in which job state, and that is where the cancellation behaviour +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` 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. + +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:`, + `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. +- 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. + +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/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..9688670 --- /dev/null +++ b/test/seidroid-review/conditions.py @@ -0,0 +1,412 @@ +#!/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 + 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 + +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, +# 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 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, + ("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, + ("cancelled", "review", "7", "true"): False, + ("success", "review", "", ""): False, + ("success", "close", "7", ""): False, + ("cancelled", "close", "7", ""): False, + }, + WITHDRAW: { + # Nothing to withdraw on a run that was not cancelled. + ("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", "true"): False, + ("cancelled", "review", "", "false"): False, + ("cancelled", "close", "7", "false"): False, + }, +} + +# 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") + +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, posted="", lenient=False): + self.state = state + self.lenient = lenient + self.values = { + "inputs.mode": mode, + "needs.guard.outputs.comment_id": comment_id, + # `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): + 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 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", + WITHDRAW, +) + + +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, 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 posted == "-" else posted) + 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} + + 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, 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" / posted={posted or 'unreported'}", + want, + 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 + # 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 + + +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..42b531f --- /dev/null +++ b/test/seidroid-review/reactions.sh @@ -0,0 +1,298 @@ +#!/usr/bin/env bash +# Runs the reaction steps of seidroid-review.yml under bash with a gh stub, and checks +# 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 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)" +WORKFLOW="$REPOROOT/.github/workflows/seidroid-review.yml" +SELECT="python3 $HERE/conditions.py --select $WORKFLOW" + +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"}}]' +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"}}]' + +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" "$CASE/calls.log" || true; } +ran() { grep -c "^$1\$" "$CASE/ran.txt" || true; } + +# 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 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" posted="$5" + local conclusion="$6" produced="$7" have_check="$8" + 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" + 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 + : > "$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" + : > "$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" "$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 + # 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" "$posted" "Withdraw the reactions on a cancelled run" + echo "$rc" > "$CASE/rc" + + 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 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 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 - failure false yes +check "left" "" "$(left)" +check "no post" 0 "$(calls post)" + +echo "== neutral earns no reaction ==" +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 - 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 - - '' 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 - success true yes +check "withdraw ran" 1 "$(ran 'Withdraw the reactions on a cancelled run')" +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 +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 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)" +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 - - '' 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 - - '' 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 - 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 - - '' no ANSWERED_AS=failure +check "left" "" "$(left)" +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 - - '' 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 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 true success true yes +check "left" "brandon:+1 brandon:-1 brandon:eyes $BOT:+1" "$(left)" +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 - failure false yes +check "left" "brandon:+1 brandon:-1 brandon:eyes" "$(left)" +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 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 - success true yes +check "left" "brandon:+1 $BOT:-1" "$(left)" +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 true success true yes +check "left" "brandon:+1 $BOT:+1 $BOT:rocket" "$(left)" +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 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 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 true success true yes STUB_POST=FAIL +check "rc" 0 "$(cat "$CASE/rc")" +check "left" "" "$(left)" +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 - - '' 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 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)" +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 false | paste -sd, -)" + +echo "== no step made a call the stub does not serve ==" +check "unstubbed calls" 0 \ + "$(grep -rh 'CALL UNSTUBBED' "$HERE/out-reactions" 2>/dev/null | wc -l | tr -d ' ')" + +echo +printf '%s\n' "${rows[@]}" +echo +echo "assertions: $pass passed, $fail failed" +[ "$fail" -eq 0 ]