fix(report-failure): dedup outage comments per run across matrix legs - #809
fix(report-failure): dedup outage comments per run across matrix legs#809tend-agent wants to merge 7 commits into
Conversation
A matrix workflow (e.g. review-reviewers, 5 legs) invokes report-failure.sh once per leg on an outage, every leg sharing one GITHUB_RUN_ID. The existing reconcile handles the create-create race (duplicate *issues*) but nothing dedups the append path, so each leg posted its own near-identical row — flooding the tend-outage issue with 5-6 comments all citing the same run. Guard the comment on the run not already being recorded (body or existing comment), then reconcile symmetrically to the issue path: keep the earliest comment citing this run, delete later duplicates. Convergent across concurrently-jittered legs.
tend-agent
left a comment
There was a problem hiding this comment.
The dedup logic is sound and the convergence argument holds (stable sort_by over an id-ascending API response makes every leg pick the same keeper). One robustness gap on the delete path: the reconcile selects comments by a bare contains("${RUN_URL}") substring, with no author or row-format scoping. Any comment that merely contains the run URL — a human quoting it while investigating, or a nightly-enrichment comment (#560) that lists this run among others — would be selected and, if it sorts after the earliest row, deleted. That contradicts the PR's "no human content is touched" invariant, since the code doesn't actually restrict deletion to the auto-generated rows.
Matching the full markdown anchor [workflow run](${RUN_URL}) instead scopes both the guard and the delete to the generated rows only, and as a bonus removes a latent prefix-collision false positive (a bare-URL contains also matches a longer run id that has this one as a prefix). The guard and the reconcile must stay consistent, so both suggestions below apply together.
|
New evidence, same defect, roughly 7x the volume. The 2026-08-04 outage (Claude weekly limit, Each of those 75 duplicate-leg rows also fired an Evidence log: https://gist.github.com/e08f6e62d6478163cb425a75648eb7e4 |
|
Coordination note from #836, which is now touching the neighbouring branch of this same script. Review there caught that #836's reconcile carries its row onto the keeper unguarded, so a same-matrix race would post a row duplicating the keeper's seed row — the same flood this PR removes from the That means the anchor check now exists twice in the file: once here on the append path, once on the create path. Leaving both copies rather than pre-factoring a helper — the two diffs are textually disjoint as they stand and a helper introduced on either branch would conflict with the other for no benefit until one lands. Whichever of the two merges second should fold them into one helper. |
…#823) ## Problem The `Trigger` column of a `tend-outage` row is the only pointer back to the work a failed run stranded. It goes blank for the one trigger where that pointer matters most, and prints `#null` when a field is missing. **`repository_dispatch` is unhandled.** `tend-mention` relays review events through a secretless job that re-posts them as a `repository_dispatch`, so the handle job runs on that event and the PR number arrives as `client_payload.pr` rather than in a `pull_request` object. The `if`/`elif` chain has no branch for it, so every failure on the relay path records `Trigger: N/A` — and a relayed review is exactly the case a maintainer can't recover from the run alone, since `tend-review` fires only on `pull_request_target` and never retries. This path is in constant use: `gh api "repos/max-sixty/tend/actions/runs?event=repository_dispatch"` returns a steady stream of `tend-mention` runs. **`workflow_run` names no run.** The ci-fix path hardcodes `REF="CI fix for workflow run"`, discarding `workflow_run.id` — the id of the CI failure the job was dispatched to fix. **Missing fields render as `null`.** `jq -r '.issue.number'` prints the literal string `null` when the field is absent, so the cell reads `#null` rather than falling back to `N/A`. ## Solution Add a `repository_dispatch` branch reading `client_payload.pr`, link the upstream run id in the `workflow_run` branch, and give every extraction `// empty` plus a `${VAR:+…}` guard so an absent field leaves the cell blank (rendered as `N/A` by the existing `${REF:-N/A}`) instead of `#null`. ## Testing `shared/steps/` has no shell test harness — shellcheck via pre-commit is the only automated gate, and it passes on the changed file. So the block was exercised directly: the REF logic was sliced out of the script (between the `# Build a one-line reference` comment and `TIMESTAMP=`) and sourced under crafted `GITHUB_EVENT_PATH` payloads, before and after. Before, on `origin/main`: ``` repository_dispatch -> N/A workflow_run -> CI fix for workflow run issues (no number) -> #null ``` After: ``` relayed review -> #815 relay w/o pr -> N/A ci-fix -> CI fix for [run 30795510450](https://github.com/max-sixty/tend/actions/runs/30795510450) ci-fix w/o id -> CI fix for workflow run PR event -> #821 PR event w/o number -> N/A issue comment -> #808 issue w/o number -> N/A schedule -> N/A ``` Adding a real harness for `shared/steps/` is worth considering separately — three of the recent outage-path fixes have all landed in scripts nothing can test — but that is a bigger change than this fix warrants, so it is not bundled here. ## Scope Separate from the other two open changes on this path, and textually disjoint from both. #818 names the *cause* of a failure (in `claude/action.yaml`); #809 dedups *rows* across matrix legs (in the `EXISTING` branch of this same script, lines 49+). This one fixes what the row *points at*, in the REF block at lines 19–29. Co-authored-by: tend-agent <270458913+tend-agent@users.noreply.github.com>
…1243 # Conflicts: # shared/steps/report-failure.sh
tend-agent
left a comment
There was a problem hiding this comment.
The guard is correct now — both earlier threads are addressed by 86f39cc, and hoisting the anchor into run_issue_anchor is the right shape: the row and the two dedup matchers can no longer drift apart.
One remaining defect on the reconcile, in the original commit rather than the incremental. The delete path lists comments with ?per_page=100 and no pagination, and GitHub returns issue comments oldest-first — so once a tracker passes 100 comments, the rows this run just posted are not in the response at all and the reconcile silently no-ops. That is the flooded issue the PR exists for: #831 reached 77 rows in a single outage, so crossing 100 on a longer one or a wider matrix is the expected case rather than the edge. The guard is unaffected — gh issue view --json comments does paginate — so only the second line of defence is capped.
--paginate alone will not fix it: gh applies --jq per page, which breaks the cross-page sort_by(.created_at) | .[1:] (each page would keep its own earliest). sort=created&direction=desc is ignored by this endpoint. --paginate --slurp refuses --jq, so the working form is --slurp piped to a downstream jq 'add | …' — which is also what rate-limit-preflight.sh already does one file over for /issues/$PAUSE/events?per_page=100.
Separately, this ships ~40 lines of racy dedup with no test, in a script every adopter runs. generator/tests/test_shared_steps.py already drives rate-limit-preflight.sh and mark-notification-read.sh against a fake gh, and #836 adds a report_failure_env fixture for this exact script — the guard (skip when the anchor is already present) and the reconcile (keep the earliest, delete the rest) both look cheap to cover once whichever of the two lands first.
How the pagination behaviour was verified
Against cli/cli#13840, which has 139 comments:
$ gh issue view 13840 -R cli/cli --json comments --jq '.comments | length'
139
$ gh api "repos/cli/cli/issues/13840/comments?per_page=100" --jq 'length'
100
$ gh api "repos/cli/cli/issues/13840/comments?per_page=100" --jq '[.[0].created_at, .[-1].created_at]'
["2026-07-10T13:54:16Z","2026-08-03T21:08:08Z"] # oldest-first; newest 39 absent
$ gh api "repos/cli/cli/issues/13840/comments?per_page=100&sort=created&direction=desc" --jq '[.[0].created_at, .[-1].created_at]'
["2026-07-10T13:54:16Z","2026-08-03T21:08:08Z"] # sort/direction ignored
$ gh api --paginate "repos/cli/cli/issues/13840/comments?per_page=100" --jq 'length'
100
39 # --jq runs per page
$ gh api --paginate --slurp "repos/cli/cli/issues/13840/comments?per_page=100" --jq 'length'
the `--slurp` option is not supported with `--jq` or `--template`
$ gh api --paginate --slurp "repos/cli/cli/issues/13840/comments?per_page=100" | jq -r 'add | length'
139
The empty case needs no extra guard: on a zero-comment issue --paginate --slurp yields [[]], so add gives [] and the filter emits nothing, exit 0.
Issue comments come back oldest-first, so the unpaginated `?per_page=100` read returned only the oldest page. Past 100 comments the rows this run and its racing sibling just posted are not in the response at all and the reconcile silently no-ops — on exactly the flooded trackers it exists for (#831 reached 77 rows in one outage). `--paginate` alone doesn't fix it: `gh` applies `--jq` per page, so each page would keep its own earliest comment and `sort_by | .[1:]` would delete the keeper. `--slurp` refuses `--jq`, so the filter moves downstream with `add` flattening the pages. Tests cover both halves of the dedup against a fake `gh`, alongside the existing `rate-limit-preflight.sh` and `mark-notification-read.sh` suites: the guard skipping a run already recorded, the reconcile converging two racing legs to one row, the anchor scoping that keeps a human comment naming the run out of the delete set, and the pagination case — which is the only one of the five that fails against the pre-fix script.
|
Both points addressed in Pagination — replied inline; took the suggestion as written. Tests — added five against a fake
Only The fake pages the comment list the way the endpoint does ( Coordination with #836 — both PRs now add a |
tend-agent
left a comment
There was a problem hiding this comment.
The pagination fix is right and the regression test is honest — I re-ran test_report_failure_reconciles_past_the_first_page against 4e93d87's script and it fails there (deleted [], expected ['140']), passes here. gh api --paginate --slurp on a zero-comment issue does return [[]] as the comment claims (checked against #879 in this repo), so add → [] → no output, exit 0.
Two gaps in what the new suite pins down, both verified by mutating the script and re-running:
- The fake accepts
--slurpwith--jq; realghexits 1 on it. That combination is the exact trap the script's comment exists to warn about, and the fake currently blesses it. - The guard's issue-body half is never exercised —
_seen_by_the_guardhardcodesbody: "", so every case reaches the guard through the comments list only. That's the minority path: on the first failed run of an outage one leg creates the issue with its row in the body, and its four siblings arrive at a tracker with no comments at all, matchable only on the body.
Suggestions inline. The second and third apply together — the keyword arg alone is inert, the parametrize alone is a TypeError.
How each gap was confirmed
Two mutations of shared/steps/report-failure.sh, each run against the suite as it stands and against the suite with the suggestions applied:
| Mutation | As it stands | With suggestions |
|---|---|---|
| jq -r "add | …" → --jq "add | …" (fold the filter back into gh) |
5 passed | 4 failed — returncode=1, stderr='the --slurp option is not supported with --jq or --template' |
guard's --jq loses .body + "\n" + |
5 passed | 1 failed — skips_a_run_already_recorded[in-the-issue-body] |
And the underlying gh behaviour, on 2.97.0:
$ gh api --paginate --slurp "repos/max-sixty/tend/issues/879/comments?per_page=100"
[[]]
$ gh api --paginate --slurp "repos/max-sixty/tend/issues/879/comments?per_page=100" --jq 'length'
the `--slurp` option is not supported with `--jq` or `--template`
$ echo $?
1
Also checked: --slurp is new to this repo — rate-limit-preflight.sh:114 pairs --paginate with a streaming .[] | select(…) filter, which is safe per-page and so isn't the precedent the earlier review cited. Nothing to fix there; the filter has no cross-page reduction.
Not flagged, for the record: _nwise in the fake is a private jq builtin, but a removal would break the tests loudly rather than silently. #857 also touches this file, but only its header comment.
… guard Two gaps in the suite added a commit ago, both found by mutating the script: The fake applied `--jq` whatever flags came with it, so `--slurp --jq` was accepted in the test and rejected in production. That combination is the trap the reconcile's comment warns about — folding the filter back into `gh --jq` is the obvious simplification, and it left all five tests green while the script died under pipefail right after posting its row, never reconciling. The fake now refuses it the way `gh` does. The guard matches `.body + "\n" + comments`, but every case reached it through the comments list, so dropping the body half from the script's `--jq` left the suite green. That half carries the first failed run of an outage: one leg takes the `else` branch and seeds the issue with its row in the body, and its siblings then hit the `EXISTING` branch against a tracker with no comments at all. Parametrized, at the cost of one extra case. Confirmed by re-running each mutation: the first now fails 4 tests, the second fails `skips_a_run_already_recorded[in-the-issue-body]`.
tend-agent
left a comment
There was a problem hiding this comment.
Both gaps from the last review are closed in bb0a92d, re-derived here rather than taken on the commit message's word — I re-applied each mutation to shared/steps/report-failure.sh against this HEAD:
| Mutation | Result on bb0a92d |
|---|---|
| jq -r "add | …" → --jq "add | …" |
4 failed, returncode=1, stderr='the --slurp option is not supported with --jq or --template' |
guard's --jq loses .body + "\n" + |
1 failed — skips_a_run_already_recorded[in-the-issue-body] only |
The fake's refusal also matches real gh on this runner (2.96.0): gh api --slurp --jq '.' … exits 1 with that message, and it is a parse-time rejection, so placing the check ahead of the subcommand dispatch is the right position for it. The body-half case is faithful too — the else branch seeds the issue through run_issue_create_and_reconcile with $ROW, which carries the anchor, so a sibling leg really does arrive at a tracker whose only match is the body.
357 passed in generator/, ci green on this commit. No further findings; not approving only because the PR is self-authored.
…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>
…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>
…1243 # Conflicts: # generator/tests/test_shared_steps.py
|
Merged
Verified the port still discriminates: with Worth noting the premise is still live: #905 currently carries exactly the flood this fixes — run |
Merging main brought #836's create-path dedup alongside this branch's run_issue_anchor helper, leaving the anchor built twice — once by the helper the row is written from, once by hand in run_issue_create_and_reconcile. Change the row's link text and the create path's guard stops matching it, silently. Route it through the helper, as this PR's coordination note said the second lander should.
Problem
When a matrix workflow fails during a bot outage, the
tend-outagetracker issue is flooded with one near-identical comment per matrix leg, all citing the same run.review-reviewershas a 5-leg matrix. During an outage this window (claude -pexiting 1 uniformly, self-resolved by 03:57Z), two failedreview-reviewersruns each posted ~5–6 comments to #808:30776520686→ 5 comments, all linking.../runs/3077652068630779959756→ 6 comments, all linking.../runs/30779959756Each comment is a one-row table differing only in a jitter-spread timestamp — the same run recorded 5–6 times.
Root cause
shared/steps/report-failure.shis invoked once per matrix leg, every leg sharing oneGITHUB_RUN_ID. The script already handles the concurrent-leg race on the create path — jittered backoff (#586) plus a self-heal reconcile that closes duplicate issues (#744) — but the append path had no dedup: every leg unconditionallygh issue comments its own row.So the create-create race (duplicate issues) was solved; the comment-append race (duplicate comments) was not. This is the same class of concurrent-matrix-leg noise the maintainer has fixed repeatedly (#586, #744, and #560 which batches enrichment into one comment per issue) — this closes the remaining gap.
Fix
Symmetric to the existing issue reconcile:
Net effect: one row per run, regardless of matrix width. Non-matrix workflows (single leg) are unaffected — the guard finds nothing, posts once, reconcile is a no-op.
Comments deleted are the bot's own auto-generated outage rows; no human content is touched.
Gate assessment
review-reviewersmatrix runs this window, both exhibiting the identical flood (~11 duplicate-run comments total), plus the defect is guaranteed to recur on any future matrix-workflow outage.Window & evidence