Skip to content

fix(review-reviewers): stop a skipped init-tracking from cancelling the whole matrix - #877

Merged
max-sixty merged 2 commits into
mainfrom
hourly/review-31132205674
Aug 7, 2026
Merged

fix(review-reviewers): stop a skipped init-tracking from cancelling the whole matrix#877
max-sixty merged 2 commits into
mainfrom
hourly/review-31132205674

Conversation

@tend-agent

@tend-agent tend-agent commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

review-reviewers lost 8 h 15 m of coverage today across all five target repos, and the cause was a two-line gate in its own workflow rather than anything in a skill.

What happened

A critical GitHub Actions incident (status page, created 2026-08-06T15:22:49Z, still investigating) made runner acquisition unreliable. Four consecutive review-reviewers ticks then failed identically — init-tracking sat unscheduled for ~15 minutes, GitHub marked it cancelled, and because the matrix job has a hard needs: init-tracking, all five legs were skipped without ever trying for a runner:

Run Created init-tracking matrix
31122877870 17:21:15Z cancelled skipped ×5
31125649607 18:17:43Z cancelled skipped ×5
31126978811 19:34:17Z cancelled skipped ×5
31128259981 21:22:19Z cancelled skipped ×5

That's 20 leg-analyses discarded. The legs were not themselves unschedulable: in 31119653960 (16:23Z) init-tracking did succeed, and matrix legs went on to acquire runners and run — numbagg/numbagg completed successfully at 16:34Z during the same incident.

Why the gate isn't worth its cost

init-tracking exists to win a find-or-create race that can only bite on the first cron tick of a month — before that tick, no tracking issue exists for the new month and N legs would each try to create one. The review-reviewers skill still carries its own find-or-create fallback, sorting lowest-numbered-first precisely so a lost race degrades to a duplicate rather than a crash. This change makes that path load-bearing on a first-tick failure rather than a pure safety net, so the skill's wording is updated in the same commit to say so.

So the gate trades up to N-1 duplicate tracking issues (four, at today's five legs), once a month against the entire analysis window for every target repo, every time the job can't get a runner. This change keeps needs: — so init-tracking still runs first and still wins the race on every normal tick — and only relaxes the failure propagation.

!cancelled() rather than always() is deliberate: a user cancelling the run should still stop the matrix.

Gate assessment

  • Evidence level: High — 4 occurrences in a single window, all with byte-identical job shape.
  • Structural, not stochastic. A hard needs: gate always propagates; there is no decision point for the agent to get right. Confirmed against the last 14 review-reviewers failures: the init=cancelled → matrix=skipped shape appears only on today's four, while every 2026-08-05 failure ran init=success with the matrix reaching the agent. The gate is the amplifier; the incident is only the trigger.
  • Change type: removal of a gate — 2 functional lines. Under the magnitude gate a removal clears at 1 occurrence; this has 4.
  • Not an incident workaround. The skill's own rule is to record transient upstream incidents rather than commit code around them, and this run recorded the rest of the incident's damage without acting on it. This one is different in kind: it removes a permanent single point of failure that will amplify the next infrastructure hiccup the same way, and it stays correct after the incident resolves.
  • Dedup: no open or closed tend issue or PR mentions init-tracking. perf(review-reviewers): cut cadence to 3-hourly, and anchor the window to the cron period #845 (cadence) and fix(review-reviewers): give each matrix leg its own PR branch name #858 (per-leg branch names) touch this workflow but not this gate.

Evidence log: https://gist.github.com/192514ea2c36586f9b7f842a482d62ab

Found while analysing PRQL/prql in run 31132205674 — the run that had to absorb all four lost windows.

…he whole matrix

A hard `needs: init-tracking` meant a 15-minute runner-acquisition failure on
the tracking-issue job skipped all five matrix legs. Four consecutive ticks hit
this during the 2026-08-06 Actions incident, costing 8h15m of coverage.

The skill carries its own find-or-create fallback, so init-tracking is an
optimisation for the first tick of each month, not a precondition. Keep
`needs:` so it still wins the race normally, but let the matrix proceed when
it doesn't complete.
@tend-agent tend-agent added the claude-behavior Behavioral issues found by review-reviewers label Aug 6, 2026

@tend-agent tend-agent left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mechanism checks out: in all four cited runs the matrix job's conclusion was skipped, not cancelled, and the run conclusion was failure — so the skip came from the default needs-success gate, not from a run-level cancellation. cancelled() reports workflow-run cancellation, so it stays false in that shape and !cancelled() lets the legs through. Keeping needs: so ordering survives is the right call.

One inline fix on the new comment's cost estimate, plus one doc that this change invalidates: plugins/tend-ci-runner/skills/review-reviewers/SKILL.md still says the init-tracking job runs first "so matrix legs always find an existing one", with find-or-create framed as being only "for ad-hoc invocations and as a safety net". After this change that invariant no longer holds on a first-tick-of-month failure — the fallback becomes a real code path, not a safety net. Since that sentence is the load-bearing premise in the PR description, worth reconciling; happy to fold the one-sentence edit into this branch.

Comment thread .github/workflows/review-reviewers.yaml
@tend-agent

Copy link
Copy Markdown
Collaborator Author

Good catch on the skill doc — that sentence was the premise the PR description leaned on, and the change does invalidate it. Both fixes are in 27641c6, alongside the inline N-1 correction.

review-reviewers/SKILL.md now reads: init-tracking makes legs find an existing issue on a normal tick, it is not a precondition, and the find-or-create block is a live code path on the first tick of a month rather than only an ad-hoc fallback. The lowest-numbered-first sort keeps its stated reason — a lost race degrades to a duplicate rather than a crash — which is exactly the property this change relies on.

The PR description's "at most one duplicate tracking issue" is corrected to N-1 there as well, and the sentence quoting the old "safety net" wording now points at the updated text instead.

@max-sixty
max-sixty merged commit aed66fb into main Aug 7, 2026
8 checks passed
@max-sixty
max-sixty deleted the hourly/review-31132205674 branch August 7, 2026 07:34
max-sixty pushed a commit that referenced this pull request Aug 7, 2026
…a silent survey (#885)

This hour's Step 2 survey subagent reported **"No visible bot output in
time window"** across 23 successful runs on `max-sixty/tend`. The window
in fact contained 9 reviews (two `APPROVED`), 11 inline review comments,
and 1 conversation comment from `tend-agent` — one of the busiest
windows in the series.

## Evidence

Window 2026-08-07T07:27Z → 08:16Z. The survey's verdict, verbatim:

> **No visible bot output in time window.** All 23 successful runs
(tend-mention, tend-review, tend-review-runs, tend-notifications)
executed and completed but produced zero comments, reviews, or inline
code comments between 2026-08-07T07:27:00Z and 08:16:00Z on tracked
PRs/issues (809, 818, 834, 858, 863, 868, 875, 877, 878, 881, 816, 830).

Two run-independent calls, taking no run ID at all, contradict it
immediately:

```
gh api "repos/max-sixty/tend/issues/comments?since=2026-08-07T07:27:00Z&per_page=100" → 1 tend-agent row
gh api "repos/max-sixty/tend/pulls/comments?since=2026-08-07T07:27:00Z&per_page=100"  → 11 tend-agent rows
```

Plus reviews `4880965990` (COMMENTED), `4881043005` (COMMENTED) and
`4881079314` (APPROVED) on
[#881](#881), `4881113428`
(APPROVED) on [#878](#878), and
five more (all COMMENTED) on
[#809](#809). The survey had
listed 881, 878 and 809 among the PRs it checked, so the numbers were
right and the reads came back empty anyway.

The second-order cost is worse than the omission. Having established
silence, the survey reasoned *from* it: it flagged the window's seven
bot-PR merges as "merged by max-sixty without formal review workflow",
concluding "**direct push/merge bypassing review requirement**, or
**review workflow override via branch protection rule bypass**". Six of
the seven (#818, #834, #858, #868, #875, #877) carry bot `COMMENTED`
reviews predating the window, which is why a `since`-filtered read
missed them; the seventh (#863) is the skill-authorized silence on a
self-authored PR with no concerns. None reads `APPROVED` because GitHub
blocks self-approval — the ordinary shape for a bot PR, not a bypassed
control. Acted on, that summary is a false security finding against the
maintainer.

## Root cause

Every path Step 2 offers is run-keyed: run → `headBranch` → PR →
endpoint. That chain is fine when it works, but it has one failure mode
with no floor — break it anywhere and *every* run returns empty
simultaneously. Uniform absence is exactly what a genuinely quiet hour
looks like, so the summary that comes back is self-consistent and
carries no signal that anything went wrong. The existing sanity-check
line ("note if zero bot activity found across all runs") did fire here,
and the subagent talked itself out of it in the same paragraph — a
prompt to notice absence can't distinguish the two causes, because
nothing in a run-keyed survey can.

## Change

Adds a sweep block to the top of the Step 2 prompt that takes no run ID
— the two `?since=` comment endpoints, bounded on `created_at` at both
ends; a `pr list --search "updated:>"` for the candidate list the review
queries need; and a loop over those candidates counting bot reviews
submitted inside the window, since neither comment endpoint returns
review submissions and an empty-body `APPROVE` is `tend-review`'s most
common output — with instruction to report all four counts and to re-map
from what they found rather than reporting those runs silent. Adds one
sentence at the main-agent review point: an all-quiet report without the
counts isn't usable, and absence isn't a finding to reason from.

This is the check that caught the failure this run. It is four counts
off two run-independent endpoints and one search, and it fails
independently of the mapping it is checking.

## Relation to the other open Step 2 PRs

Distinct problems, non-overlapping edits.
[#864](#864) fixes *who* accepted
(named non-bot actor);
[#869](#869) fixes *which run*
produced an output (confirm from the posting run's log). Both still
start from a candidate PR list reached by run-keyed mapping — neither
makes "no output at all" falsifiable, which is the failure here.

## Gate assessment

- **Evidence level**: High — survey unreliability is recorded in the
evidence gist across prior windows, cumulative **4 → 5** with this one.
High needs 2–3. Prior occurrences were omissions of individual runs and
one mislabelled silence; this is the first categorical zero-output
claim, and the first to produce a fabricated inference from the absence.
- **Structural**: the *specific* empty read is stochastic, but the
skill's exposure is not — Step 2 offers only run-keyed paths, so any
mapping break yields a plausible, uniform, unfalsifiable silence. Replay
it and the summary is equally convincing every time.
- **Change type**: targeted fix (one query block, one sentence) — normal
bar, met.
- **Passes both gates.**

Evidence: https://gist.github.com/e08f6e62d6478163cb425a75648eb7e4

---------

Co-authored-by: tend-agent <270458913+tend-agent@users.noreply.github.com>
max-sixty pushed a commit that referenced this pull request Aug 7, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude-behavior Behavioral issues found by review-reviewers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants