feat(seidroid-review): widen the trigger to every comment event, and let a caller name it

.github/workflows/workflow-test-self.yml

@@ -1,20 +1,38 @@
 name: Workflow tests
-# The shell and jq inside seidroid-review.yml, run against a gh stub. Nothing here
-# reaches the GitHub API, so this needs no token and no permissions.
+# The shell and jq inside seidroid-review.yml, run against a gh stub, and its job
+# conditions evaluated against synthetic payloads. Nothing here reaches the GitHub
+# API, so this needs no token and no permissions.
 #
-# Two steps, one harness. Placement records which thread each posted comment
-# replaced and the resolve step closes on that record, so the pair is the behaviour
-# worth testing rather than either half.
+# Three jobs, so each check in the list names the step it covers. A job that ran two
+# harnesses would report one name for two things, and one check would go red for the
+# other's failure.
+#
+# `Place findings and resolve threads` covers a pair of steps rather than either half:
+# placement records which thread each posted comment replaced, and the resolve step
+# closes on that record. `The reaction steps` covers which of the three runs in which
+# job state, and what each leaves on the trigger comment. `Guard the request` covers
+# the admission path, from the event the workflow accepts to the collection the
+# acknowledgement reaches.
 on:
   pull_request:
     paths:
       - '.github/workflows/seidroid-review.yml'
+      # run-guard.sh asserts what ai-assistant.yml claims of the same comment, so an
+      # edit there can break an invariant stated here. Without this the break lands
+      # on the next unrelated pull request that touches the file above, pointing at
+      # the wrong change.
+      - '.github/workflows/ai-assistant.yml'
       - '.github/workflows/workflow-test-self.yml'
       - 'test/seidroid-review/**'
   push:
     branches: [ main ]
     paths:
       - '.github/workflows/seidroid-review.yml'
+      # run-guard.sh asserts what ai-assistant.yml claims of the same comment, so an
+      # edit there can break an invariant stated here. Without this the break lands
+      # on the next unrelated pull request that touches the file above, pointing at
+      # the wrong change.
+      - '.github/workflows/ai-assistant.yml'
       - '.github/workflows/workflow-test-self.yml'
       - 'test/seidroid-review/**'
 permissions:
@@ -35,9 +53,6 @@ jobs:
       - name: Run the placement and resolve harness
         run: test/seidroid-review/run.sh
 
-  # Its own job, so each check in the list names the step it covers. A job that runs
-  # both harnesses would report one name for two things, and the placement check would
-  # go red for a reaction.
   reactions:
     name: The reaction steps
     runs-on: ubuntu-latest
@@ -56,3 +71,18 @@ jobs:
       # lives and which the shell harness above cannot see.
       - name: Check the step conditions
         run: python3 test/seidroid-review/conditions.py .github/workflows/seidroid-review.yml
+
+  guard-admission:
+    name: Guard the request
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v7
+      - name: Set up Python
+        uses: actions/setup-python@v7
+        with:
+          python-version: '3.x'
+      - name: Install the YAML reader
+        run: python3 -m pip install --quiet pyyaml
+      - name: Run the guard harness
+        run: test/seidroid-review/run-guard.sh

test/seidroid-review/.gitignore

@@ -9,3 +9,11 @@ ack.sh
 answer.sh
 withdraw.sh
 out-reactions/
+# Written by run-guard.sh: the five steps it extracts from the workflow. Two of
+# them are steps reactions.sh also extracts, under names of their own so neither
+# harness can overwrite the other's extraction.
+refuse.sh
+parse.sh
+admit.sh
+guard-ack.sh
+guard-answer.sh

test/seidroid-review/README.md

@@ -1,40 +1,48 @@
 # Workflow tests
 
-Two harnesses over `.github/workflows/seidroid-review.yml`. Both read the steps out
-of the YAML on every run, so neither can pass against a stale copy.
+Three harnesses over `.github/workflows/seidroid-review.yml`. All three read the
+steps out of the YAML on every run, so none can pass against a stale copy.
 
 ```sh
 test/seidroid-review/run.sh        # placement and thread resolution
 test/seidroid-review/reactions.sh  # the three reaction steps
+test/seidroid-review/run-guard.sh  # the guard, and the reaction collection
 python3 test/seidroid-review/conditions.py .github/workflows/seidroid-review.yml
 ```
 
-# `Place findings on the code` and `Resolve the threads this review closed`
+Each needs `bash`, `jq`, and `python3` with PyYAML, and exits non-zero on the
+first failed assertion count.
 
-Runs both steps under `bash`, against a `gh` stub, and checks what they posted,
-counted and closed.
+`extract.py` is shared. It reads a step's `run:` block and a workflow-level env
+key out of the YAML, by step name or step id.
+
+`reactions.sh` and `run-guard.sh` both extract `Acknowledge the trigger` and
+`Answer the request`, and they ask different things of them: `reactions.sh` asks
+which step runs in which job state and what it leaves on the comment,
+`run-guard.sh` asks which REST collection the URL reaches. Each writes its own
+extraction — `ack.sh` and `answer.sh` against `guard-ack.sh` and
+`guard-answer.sh` — so running both cannot have one overwrite the other.
 
-The run needs `bash`, `jq`, and `python3` with PyYAML. It exits non-zero on the
-first failed assertion count and prints a table of one row per case.
+## `Place findings on the code` and `Resolve the threads this review closed`
+
+Runs both steps under `bash`, against a `gh` stub, and checks what they posted,
+counted and closed. It prints a table of one row per case.
 
 Both steps are in one harness because they are one behaviour. Placement records
 which thread each posted comment replaced; the resolve step closes a thread on
 finding its id in that record. A harness that ran only one of them could not
 tell whether the record it wrote is the record the other reads.
 
-## How it works
-
-`extract.py` reads a step's `run:` block and the workflow's `FINDING_MARKER` out
-of the YAML on every run, so the harness tests the file as it stands. It runs
-twice, once per step, and the two markers are asserted equal: placement stamps a
-comment with it and the resolve step recognises a thread by it.
+The extractor runs twice, once per step, and the two `FINDING_MARKER` readings
+are asserted equal: placement stamps a comment with it and the resolve step
+recognises a thread by it.
 
 `bin/gh` goes on `PATH` ahead of the real `gh`. It logs every call, serves
 fixture JSON through the step's own `jq`, keeps the request body the step sent,
 and decides per case whether a call succeeds. `STUB_*` variables in `run_case`
 and `run_resolve` select the fixtures and the answers.
 
-## The fixtures
+### The fixtures
 
 `fx/files*.json` are `GET /compare` responses. One JSON object each: compare
 paginates its commits, and a second page carries no `files` key, so the step
@@ -59,7 +67,7 @@ because every body has to open with the marker the workflow defines now: two
 pages, and four threads that fail this step's own tests — the other identity, a
 foreign account, no marker, and a marker quoted mid-body.
 
-# The reaction steps
+## The reaction steps
 
 `reactions.sh` runs `Acknowledge the trigger`, `Answer the request` and
 `Withdraw the reactions on a cancelled run` against `bin-reactions/gh`, which keeps
@@ -89,7 +97,7 @@ later cancellation. And a run cancelled before it reached `Answer the request` t
 only the eyes: a thumb on the comment then belongs to an EARLIER run, whose verdict may
 still stand.
 
-# The step conditions
+## The step conditions
 
 `conditions.py` covers what a shell harness cannot see. A step condition decides which
 reaction step runs in which job state, and that is where the cancellation behaviour
@@ -118,3 +126,65 @@ step is not invisible to them, and both search the **whole step** rather than on
 
 No check needs telling where to look. A check that has to be pointed at a step is not
 stated over the file.
+
+## The guard, and the two steps that react
+
+Runs the request-admission path under `bash` against a `gh` stub of its own, and
+evaluates the shipped job conditions against synthetic event payloads. Five
+steps are read: `Refuse an event this workflow does not handle`, `parse`, `Admit
+the request`, `Acknowledge the trigger` and `Answer the request`.
+
+`gha.py` covers what a script harness cannot see. A job condition and a step's
+`env:` mapping are GitHub expressions, and both decide which payload field a
+request is read from, so both are evaluated here rather than restated. It models
+four GitHub semantics the conditions rest on — case-insensitive string
+comparison, `||` and `&&` yielding one operand each, **both short-circuiting**,
+and `contains` over an array testing membership — and `--selftest` checks each
+one. That model is read from GitHub's published expression semantics: nothing in
+this directory calls a runner.
+
+Short-circuiting is the one to be careful with. The runner's Or and And nodes
+return on the first truthy or falsy operand and never evaluate the rest, so a
+`fromJSON` an operand nothing reaches would refuse never runs. A model that
+evaluated eagerly reports a failure the runner does not have, and one assertion
+here stated the opposite of what a real event does before this was modelled.
+
+Four modes:
+
+```sh
+gha.py <workflow> <job> <context.json>          # the job's if:, as true or false
+gha.py --env <workflow> <step> <key> <ctx.json> # what a step's env key resolves to
+gha.py --input <workflow> <input> <field>       # a declared workflow_call input field
+gha.py --selftest                               # the expression model itself
+```
+
+One group reaches outside this file. `ai-assistant.yml` answers the same comments
+and reserves the exact `@seidroid review` body for the reviewer, so the last group
+evaluates that workflow's own reply condition beside the parse and records which
+tool answers each body. Every body is checked on all three comment events, because
+the assistant has a branch each and this workflow now answers all three: a helper
+naming one event would measure the division on the path that already had it and
+infer the two this workflow adds. Two bodies both tools answer; the group says
+which and why.
+
+`ai-assistant.yml` is in `workflow-test-self.yml`'s `paths:` filter for that
+reason. Without it an edit there breaks an invariant stated here, and the break
+lands on the next unrelated pull request that touches `seidroid-review.yml`.
+
+`bin-guard/gh` logs every call, serves the answer the case chose through the
+step's own `--jq` filter, and tells the fork check from the label check by the
+filter each sends. A failed read prints nothing and exits non-zero, which is the
+shape `Admit the request` is written against: it captures stdout, so an empty
+capture is what tells the fork check and the once-per-PR gate that nobody
+answered. `bin/gh` beside it files an error body instead, because the placement
+step reads one.
+
+### The fixtures
+
+There are none. A guard case turns on six payload fields and five API answers,
+so each is built in the run from `STUB_*` and context arguments, where the case
+that chose it can be read beside the assertion it drives.
+
+`STUB_TEAM`, `STUB_ORIGIN`, `STUB_LABELS`, `STUB_REVIEWS`, `STUB_COMMENTS` and
+`STUB_REACTIONS` choose what the stub answers; `FAIL` on any of them is a read
+that nobody answered.

test/seidroid-review/bin-guard/gh

@@ -0,0 +1,115 @@
+#!/usr/bin/env bash
+# gh stub for the guard steps: logs every call, serves the answer the case chose,
+# and runs the step's own --jq filter over it.
+#
+# A failed read prints nothing and exits non-zero, which is the shape `Admit the
+# request` is written against: it captures stdout, so an empty capture is what
+# tells the fork check and the once-per-PR gate that nobody answered. The
+# placement stub beside this one files an error body instead, because that step
+# reads one.
+#
+# Five reads reach here. The team membership read and the once-per-PR reads are
+# told apart by their path; the fork check and the label check share a path and
+# are told apart by the filter each sends.
+log() { printf '%s\n' "$*" >> "$STUB_LOG"; }
+
+argv=("$@")
+joined="$*"
+
+filter=""
+n=${#argv[@]}
+for ((i = 0; i < n; i++)); do
+  [ "${argv[i]}" = "--jq" ] && filter="${argv[i + 1]}"
+done
+
+serve() { # fixture-json
+  printf '%s' "$1" | jq -r "$filter"
+  exit 0
+}
+
+refuse() { # what-failed
+  log "CALL $1 FAILED"
+  echo "gh: the stub was told this read fails" >&2
+  exit 1
+}
+
+# Ahead of the comment reads below: an issue comment's reactions path carries
+# /issues/ and comments too, and the first matching case wins.
+case "$joined" in
+  *"/reactions"*)
+    method=GET
+    for ((i = 0; i < n; i++)); do
+      [ "${argv[i]}" = "-X" ] && method="${argv[i + 1]}"
+    done
+    for a in "${argv[@]}"; do case "$a" in repos/*/reactions*) url="$a" ;; esac; done
+    log "CALL reaction $method ${url:-none}"
+    [ "${STUB_REACTIONS:-none}" = "FAIL" ] && refuse reaction
+    case "$method" in
+      GET)
+        case "${STUB_REACTIONS:-none}" in
+          eyes) serve '[{"id":1,"content":"eyes","user":{"login":"github-actions[bot]"}}]' ;;
+          *)    serve '[]' ;;
+        esac
+        ;;
+      *) exit 0 ;;
+    esac
+    ;;
+esac
+
+case "$joined" in
+  *"/teams/"*"/memberships/"*)
+    for a in "${argv[@]}"; do case "$a" in */memberships/*) who="${a##*/}" ;; esac; done
+    log "CALL membership $who"
+    [ "${STUB_TEAM:-active}" = "FAIL" ] && refuse membership
+    serve "{\"state\":\"${STUB_TEAM:-active}\"}"
+    ;;
+esac
+
+case "$joined" in
+  *"/pulls/"*"/reviews"*)
+    log "CALL reviews"
+    [ "${STUB_REVIEWS:-none}" = "FAIL" ] && refuse reviews
+    case "${STUB_REVIEWS:-none}" in
+      blocked) serve "[{\"id\":11,\"state\":\"CHANGES_REQUESTED\",\"body\":\"${VERDICT_MARKER}a block\"}]" ;;
+      *)       serve '[]' ;;
+    esac
+    ;;
+esac
+
+case "$joined" in
+  *"/issues/"*"/comments"*)
+    log "CALL comments"
+    [ "${STUB_COMMENTS:-none}" = "FAIL" ] && refuse comments
+    case "${STUB_COMMENTS:-none}" in
+      verdict) serve "[{\"id\":22,\"user\":{\"type\":\"Bot\"},\"body\":\"${VERDICT_MARKER}a verdict\"}]" ;;
+      *)       serve '[]' ;;
+    esac
+    ;;
+esac
+
+case "$joined" in
+  *"/pulls/"*)
+    # The fork check names head.repo.id in its filter; the label check names labels.
+    case "$filter" in
+      *head.repo.id*)
+        log "CALL origin"
+        [ "${STUB_ORIGIN:-same}" = "FAIL" ] && refuse origin
+        case "${STUB_ORIGIN:-same}" in
+          same) serve '{"head":{"repo":{"id":1}},"base":{"repo":{"id":1}}}' ;;
+          fork) serve '{"head":{"repo":{"id":2}},"base":{"repo":{"id":1}}}' ;;
+          null) serve '{"head":{"repo":null},"base":{"repo":{"id":1}}}' ;;
+        esac
+        ;;
+      *labels*)
+        log "CALL labels"
+        [ "${STUB_LABELS:-}" = "FAIL" ] && refuse labels
+        names="$(jq -nc --arg list "${STUB_LABELS:-}" \
+          'if $list == "" then [] else ($list | split(",") | map({name: .})) end')"
+        serve "{\"labels\":$names}"
+        ;;
+    esac
+    ;;
+esac
+
+log "CALL unhandled $joined"
+exit 1

test/seidroid-review/bin-reactions/gh

@@ -27,15 +27,15 @@ done
 
 case "$verb:$path" in
   GET:*/reactions)
-    log "CALL list"
+    log "CALL list $path"
     [ "${STUB_LIST:-ok}" = FAIL ] && exit 1
     # Through the step's own filter, so the filter is under test and not the harness's
     # idea of it.
     if [ -n "$filter" ]; then jq -r "$filter" < "$STUB_STATE"; else cat "$STUB_STATE"; fi
     ;;
   DELETE:*/reactions/*)
     rid="${path##*/}"
-    log "CALL delete $rid"
+    log "CALL delete $rid $path"
     [ "${STUB_DELETE:-ok}" = FAIL ] && exit 1
     jq -e --argjson rid "$rid" 'any(.[]; .id == $rid)' < "$STUB_STATE" > /dev/null || {
       log "  no such reaction $rid"; exit 1; }
@@ -45,7 +45,7 @@ case "$verb:$path" in
   POST:*/reactions)
     content=""
     for f in "${fields[@]:-}"; do case "$f" in content=*) content="${f#content=}" ;; esac; done
-    log "CALL post $content"
+    log "CALL post $content $path"
     [ "${STUB_POST:-ok}" = FAIL ] && exit 1
     # Idempotent per (user, content): the API returns the reaction already there
     # rather than adding a second one.

test/seidroid-review/extract.py

@@ -1,10 +1,11 @@
 import sys, yaml
 path, step, out = sys.argv[1], sys.argv[2], sys.argv[3]
+key = sys.argv[4] if len(sys.argv) > 4 else "FINDING_MARKER"
 d = yaml.safe_load(open(path, encoding="utf-8"))
 for job in d["jobs"].values():
     for s in job.get("steps", []):
-        if s.get("name") == step:
+        if s.get("name") == step or s.get("id") == step:
             open(out, "w", encoding="utf-8").write(s["run"])
-            print(d["env"]["FINDING_MARKER"])
+            print(d["env"][key])
             sys.exit(0)
 sys.exit("step not found: " + step)