tend-review CI monitor polls `--required`, misses in-flight `tests` failures on opened PRs

[prql-bot]

Problem

The tend-review run for a new PR triggers on pull_request_target: opened and runs in parallel with the tests workflow. Its Step 6 (CI monitoring) polls gh pr checks <number> --required in a loop and exits as soon as no required check is still pending/queued/in_progress.

In this repo, the only status context that reports as "required" at polling time is pre-commit.ci - pr. The branch's real required omnibus, check-ok-to-merge (defined in tests.yaml with if: always() and a long needs: list), is not registered as a check-run until its dependencies start completing. So the review's polling loop sees only pre-commit.ci - pr, watches it pass in a few minutes, and exits reporting "CI passed" while test-rust and siblings are still running.

Evidence

Most recent occurrence: PR #5807 (rust toolchain 1.93.1 → 1.94.1).

• 11:12:32Z — tend-review run 24562181409 started.
• 11:17:10Z — review session ended. Session JSONL shows gh pr checks 5807 --required returned only pre-commit.ci - pr; check-ok-to-merge was not in the response (0 occurrences in the tool-result log vs. 3 for pre-commit.ci - pr).
• 11:18:59Z — first test-rust failure (a new clippy::manual_saturating_arithmetic lint in Rust 1.94).
• 11:40:35Z — check-ok-to-merge check-run finally registered on commit 8ff52aff, 23 minutes after the review had finished.

The bot's review therefore signalled "all required checks passed" while the PR was actually broken. A human maintainer had to flag it; reconstructing the root cause took 4 subsequent tend-mention runs on the PR thread.

Proposed fix

Add a short section to .claude/skills/running-tend/SKILL.md telling future bot sessions on this repo not to use gh pr checks --required in the review monitoring loop. Poll without --required so the loop waits for every in-flight tests job to finish.

--- a/.claude/skills/running-tend/SKILL.md
+++ b/.claude/skills/running-tend/SKILL.md
@@ -15,3 +15,32 @@
 - Main CI workflow: `tests` (watched by tend-ci-fix)
 - Dependency management: Dependabot (tend-weekly is disabled)
 - Automerge: `pull-request-target.yaml` auto-merges single-commit `prql-bot` PRs
   once CI passes
+
+## CI monitoring after review
+
+When polling CI after submitting a review (Step 6 of `tend-ci-runner:review`),
+**do not use `gh pr checks --required`** on this repo. The required check
+`check-ok-to-merge` is an `alls-green` omnibus defined in `tests.yaml` with
+`if: always()` and a long `needs:` list; its check-run is not registered
+until its dependencies (`test-rust`, `test-js`, etc.) complete. `tend-review`
+runs triggered by `pull_request_target: opened` run in parallel with `tests`,
+so at polling time `gh pr checks --required` sees only `pre-commit.ci - pr`;
+the loop exits as soon as it passes and misses in-flight test failures.
+
+Poll without `--required` instead:
+
+```bash
+for i in $(seq 1 15); do
+  sleep 60
+  gh pr checks <number> 2>&1 | grep -v "/runs/$GITHUB_RUN_ID/" \
+    | grep -q 'pending\|queued\|in_progress' || {
+    gh pr checks <number>
+    exit 0
+  }
+done
+echo "CI still running after 15 minutes"
+exit 1
+```
+
+The wider poll catches every `tests` job; budget up to 15 minutes since the
+full `tests` matrix runs longer than `pre-commit.ci - pr` alone.

I can't apply this myself — the CI sandbox marks .claude/skills/ as a sensitive path and blocks writes from both the Edit tool and Bash, even in a worktree under $TMPDIR. Same block was noted in #5802 for a different skill update; that diff ended up needing a maintainer to land it too.

Gate assessment

• Evidence level: Critical (wrong outcome — bot signalled CI passed while PR was broken).
• Structural: Yes — any pull_request_target: opened-triggered review on this repo will see the same race.
• Occurrences: 1 this cycle; Critical + structural passes Gate 1 at 1.
• Magnitude: Targeted addition (~29 lines in an existing skill file).

[prql-bot]

Verified the root cause from the session log and the current tests.yaml: check-ok-to-merge is an alls-green omnibus with if: always() and a long needs: list, so its check-run isn't registered until its dependencies start completing. The tend-review run for #5807 (run 24562181409) started at 11:12:32Z and exited at 11:17:10Z, while the tests run on the same commit failed later (conclusion: failure). The proposed diff — polling without --required in .claude/skills/running-tend/SKILL.md — matches the evidence and doesn't duplicate upstream tend-ci-runner:running-in-ci guidance (which uses --required as a default that this repo's omnibus structure breaks).

Couldn't apply the patch automatically: the CI sandbox lists /home/runner/work/prql/prql/.claude/skills under denyWithinAllow, and the Edit tool is blocked with a permissions error. This is a deliberate policy gate for skill changes, so I'm leaving the diff in the issue body for a maintainer to land rather than bypassing the sandbox.

[max-sixty]

is there a way we can change the polling to not miss the "required task which hasn't yet started"?

can we move this issue to the tend repo / open one there? it seems like an upstream problem

[prql-bot]

is there a way we can change the polling to not miss the "required task which hasn't yet started"?

A few options, in order of how much they'd change:

1. Poll workflow runs instead of check runs. gh run list --commit <sha> --json name,status,conclusion sees runs the moment they're queued, so it doesn't depend on a check-run being registered. This sidesteps the whole alls-green/if: always() race.
2. Resolve required contexts from branch protection first, then poll for those specific contexts by name (gh pr checks | jq 'select(.name==...)'). Avoids --required's "what's registered right now" snapshot, but adds API surface and assumes the bot can read branch protection.
3. Minimum wait before first poll (e.g. sleep 90s so check-ok-to-merge has time to register). Cheapest, but fragile — arbitrary constant against a variable matrix.

Option 1 seems like the cleanest upstream default; the proposed repo-local diff (drop --required) is a strict subset of that.

can we move this issue to the tend repo / open one there? it seems like an upstream problem

Agreed — the --required default lives in tend-ci-runner:review Step 6, and any repo that uses an alls-green omnibus (a fairly common pattern) will hit the same race. I don't have write access to max-sixty/tend to transfer it, and my scope here is limited to the PRQL org, so I'll leave opening the upstream issue to you.

[max-sixty]

open the upstream issue with option 1

[prql-bot]

Opened upstream: max-sixty/tend#305. Filed with Option 1 (poll workflow runs via gh run list --commit <sha> instead of gh pr checks --required) as the proposed fix, along with the tradeoffs (waits for all runs, not just required ones; optional branch-protection filter to narrow it) and alternatives considered. The PRQL #5807 incident is cited as the downstream evidence.

[prql-bot]

Resolved upstream in tend.

The CI monitoring loop in tend-ci-runner:running-in-ci (now shipped with tend 0.1.0) polls statusCheckRollup instead of gh pr checks --required, filters out the current run's own check, and includes a 30s grace re-check so late-registering omnibus checks like check-ok-to-merge are picked up. The comment block on that loop explicitly calls out this repo's scenario — the alls-green omnibus registers only once its needs: list reaches terminal, so --required would see only pre-commit.ci - pr and exit early. The rollup sees matrix jobs as IN_PROGRESS while they run, so the loop correctly waits for them.

The review skill's Step 6 now defers to that loop rather than inlining its own --required poll, so new tend-review runs on this repo will wait for the full tests matrix. Closing.