fix(running-in-ci): make the CI-monitor poll cap terminal - #876
fix(running-in-ci): make the CI-monitor poll cap terminal#876tend-agent wants to merge 2 commits into
Conversation
pending() counts advisory checks, so a repo with a long non-required matrix can never reach 0 inside the 9-minute cap. Two tend-review sessions re-entered the loop three times each rather than taking the step 4 terminal branch.
8731f97 to
e63697c
Compare
tend-agent
left a comment
There was a problem hiding this comment.
Self-authored, so this is a COMMENT rather than an approval. The rule itself lands — "the cap is the whole poll budget, not one attempt" is exactly the sentence the two re-entering sessions were missing. Two things on the wording, plus a merge-order note.
Required-ness isn't derivable from the recipe. The new text asks the unverified-checks comment to name "whether it's required", but the poll deliberately avoids --required (the block comment above pending() explains why) and the rollup it does read carries no required flag. So the step asks for a fact nothing in the section surfaces, and a session will improvise it — the failure mode this skill file spends most of its length guarding against. Naming the source costs a clause; the inline suggestion adds it, along with the caveat the recipe already establishes (an omnibus that hasn't registered yet won't appear in --required output, and is the one most likely to be required).
"A second pass buys nothing" is broader than the mechanism behind it. The mechanism given — pending() counting an hour-long advisory matrix — makes re-entry futile in that case, which is the case in the evidence. It doesn't cover a session gated on its own push whose required suite runs 12 minutes: there a second pass would settle it and let the gated follow-up (fix the red, dismiss the approval) fire, where ending ships a comment and leaves the push unverified. That's not a regression — the old text ended at the cap too — and the trade is defensible given the sessions burning 28 minutes on advisory jobs. But stating it as an unconditional "buys nothing" makes the rule rest on a claim that isn't true in the required-check case; the suggestion drops that clause and keeps the structural reason, which carries the rule on its own.
Merge-order note: #870 inserts a subsection immediately after this step and carries the old step-4 text as a context line, so whichever of the two lands second will need a trivial rebase.
…onal claim Review feedback: step 4 asked the unverified-checks comment to mark each check required, but the poll reads statusCheckRollup, which carries no required flag — so the fact had no source and a session would improvise it. Point at `gh pr checks --required` and carry over the caveat the recipe already establishes about unregistered omnibus checks. Also drop "a second pass buys nothing": true for the advisory matrix in the evidence, not for a session gated on its own push whose required suite would settle in a second pass. The structural reason carries the rule alone.
…nst tend before filing upstream (#891) ## Problem Two dedup blocks were blind in two different ways, and the cited duplicate needed both fixed. **State filter.** `review-runs` Step 5 and `review-reviewers` Step 4 both deduped against PRs with `gh pr list --state open`. A merged PR is never returned by that query, so a finding whose fix already landed reads as undeduped and gets filed again. `running-in-ci`'s PR-creation dedup recheck already gets this right ("with `--state all` so closed and merged siblings show up"); these two recipes contradicted it. **Repo scope.** `review-runs` is a generated workflow ([`generator/src/tend/config.py:24`](https://github.com/max-sixty/tend/blob/f65f49f/generator/src/tend/config.py#L24) lists it in the enabled set), so it runs in each adopter's checkout and an unqualified `gh pr list` returns *the adopter's* PRs. Step 6 routes bundled-skill defects upstream to tend, but neither Step 5 nor any of `running-in-ci`'s dedup recipes — all local-repo — told the agent to dedup in the target repo before filing there. `--state all` alone does not close this: the adopter's PR list never contained the upstream fix at any state. `review-reviewers` is unaffected by the second half. It runs in `max-sixty/tend` and files onto tend, so its unqualified `gh pr list` already resolves to the right repo; only the state filter was wrong there. This bites hardest on tend specifically, because of the pinning model: adopters call `max-sixty/tend/<harness>@X.Y.Z`, so a merged skill fix stays dormant on their repos until the next release tags. The bug keeps reproducing after the fix merges — which is exactly the window in which the analysis legs are looking at it, and exactly when the dedup queries are blind to the fix. ## What happened `max-sixty/cargo-affected`'s `tend-review-runs` run [31160677649](https://github.com/max-sixty/cargo-affected/actions/runs/31160677649) (08:11:33Z → 08:21:41Z) hit the `| last` evidence-log mis-selection: it appended ~12 KB of run evidence into the nightly's unrelated comment on target [#73](max-sixty/cargo-affected#73), noticed on its post-verify read, restored comment `5188771252`, and re-appended to the real log `5150650688`. Good recovery. It then filed [#883](#883) upstream, whose "Proposed fix" is a `## Run ` heading predicate on the comment selector. [#875](#875) merged that exact fix at 07:34:40Z — 46 minutes before the issue was filed — as `test("^## Run [0-9]")` on the same selector, in the same file. #883 is a duplicate of a merged PR. The run made three dedup queries before filing (`gh issue list --state all --search "tracking issue comment append"`, a broader `gh issue list --state all` title regex, and a final `gh issue list --state open` recheck). All three were `gh issue list`, which never returns PRs — and all three ran against `max-sixty/cargo-affected`. Even had it run Step 5's PR line verbatim, it would not have returned #875, for both reasons: the state filter excluded merged PRs, and the query's repo was the adopter's, not tend's. ## The fix - Both skills: `gh pr list --state open` → `--state all`, projecting `state,mergedAt` so a merged hit is legible. - `review-runs` only: add the cross-repo pair (`gh pr list`/`gh issue list --repo max-sixty/tend --state all`) so a finding heading upstream under Step 6 is deduped against tend first. - `review-runs` only: the pinning note is scoped to the upstream repo, since in that skill the reader is the adopter and the local `gh pr list` above it has nothing to do with pinned refs. `review-reviewers` keeps the original wording, where tend is the reader and "on adopters" is the correct direction. Both added commands were run against this repo to confirm they parse and return the expected shape. ## Gate assessment - **Evidence level**: High. **Occurrences: 1** direct, verified end to end (session log, both dedup query sets, #875's merge time and diff, #883's body). - **Structural, not stochastic.** `gh pr list --state open` deterministically cannot return a merged PR, and a query scoped to the adopter's repo deterministically cannot return a tend PR; replayed ten times it misses #875 ten times. There is no decision point. - **Change type**: targeted fix — query lines plus one sentence naming the pinning consequence. It brings both recipes into line with a rule the same plugin already states in `running-in-ci`, rather than introducing new policy. - **Why act at one occurrence**: the six-PR batch merged at 07:34:40Z ([#875](#875), [#834](#834), [#868](#868), [#818](#818), [#877](#877), [#858](#858)) is all unreleased, so six distinct bugs remain observable on every adopter until the next release. The first analysis leg after that batch produced the first duplicate. The exposure is six-wide and standing, not one-off. ## Not covered by No open PR touches either dedup block. [#845](https://github.com/max-sixty/tend/pull/845)/[#850](https://github.com/max-sixty/tend/pull/850)/[#838](https://github.com/max-sixty/tend/pull/838) are the run-window cluster; [#849](#849), [#856](#856), [#857](#857), [#864](#864), [#869](#869), [#876](#876), [#809](#809), [#837](#837), [#821](#821), [#836](#836) are elsewhere. #883 stays open as a maintainer call — it is redundant with #875 but the option-2 half of its body (finish the gist migration for `review-runs`) is not. Evidence: https://gist.github.com/dca23a6e6a0d8cae2665944ba31676fb --------- Co-authored-by: tend-agent <270458913+tend-agent@users.noreply.github.com>
Problem
The gated CI poll in CI Monitoring caps at ~9 minutes, but nothing says the cap is the whole budget. Two
tend-reviewsessions onmax-sixty/worktrunkhave now hit the cap and simply run the loop again — three full passes each, ~28 minutes of post-approval polling — because re-entering reads as diligence rather than as waiting on something that already proved it won't settle.It can't settle.
pending()deliberately reads the wholestatusCheckRolluprather than--required, so it counts advisory jobs too. worktrunk'snightlyworkflow runsfeature-powersetandbenchmarksfor 80+ minutes on a PR head — the repo's own overlay documents the benchmark job's runtime — so on any PR that triggers thempending()structurally cannot return 0 inside a 9-minute cap, no matter how many times the loop is re-entered.Evidence
feature-powerset(started 08:49:11Z, still running an hour later)feature-powerset,codecov/patch(never registered)In run 31086241862 every required check was terminal by 09:00:25Z; the remaining 16 minutes of polling watched one advisory job. The session's own log shows it saw
feature-powerset IN_PROGRESSon all nine iterations of the final pass and then ended with the reasoning"One non-required nightly job, feature-powerset, was still running when I hit the polling cap (~28 min) — I didn't wait it out; it's advisory and a failure there couldn't stem from a comment-only diff."— a sound call, but it stayed in the session log. Step 4 asks for that as a comment; nothing was posted, so the PR carries an approval with no record of what went unverified. The 30833412090 session did post the comment, so the two runs resolved the same state differently.Change
Step 4 now says the cap is terminal, gives the structural reason (
pending()counts advisory jobs, so a long matrix can never reach 0), and specifies what the unverified-checks comment should carry — each pending check, marked required or advisory, withgh pr checks --requirednamed as the source since the rollup the poll reads carries no required flag. An advisory-only tail becomes one cheap line rather than a judgment call about whether to say anything at all.Gate assessment
pending()counts non-required checks by design (the recipe explains why it avoids--required), so any repo with a long advisory matrix reproduces this on every gated poll.Not covered elsewhere
#821 addresses the pre-APPROVE rollup guard in
review/SKILL.md(provenance of a stale red when the cap expires); this is the post-approve dismiss-on-red poll inrunning-in-ci, where a terminal rule already exists and is being skipped rather than improvised. #870 adds a subsection after step 4 about reviews landing mid-poll and does not touch the step itself.Evidence log: https://gist.github.com/a88c03f4d0c3fb1791060ff3dd97d1c4