fix(report-failure): dedup outage comments per run across matrix legs - #809
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
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
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
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
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.
…947) Found by `review-reviewers` analysing `max-sixty/worktrunk` ([run 31510773013](https://github.com/max-sixty/tend/actions/runs/31510773013)). Evidence log: https://gist.github.com/a88c03f4d0c3fb1791060ff3dd97d1c4 ## What happened Worktrunk's Claude subscription hit its weekly limit during the window, and two runs failed with `You've hit your weekly limit · resets 2am (UTC)` (visible in each session JSONL, zero tokens billed on the first): | Run | Workflow | Agent outcome | Recorded on the tracker? | |---|---|---|---| | [31504411090](https://github.com/max-sixty/worktrunk/actions/runs/31504411090) | `tend-mention` | `claude -p` exit 1 | yes — filed [worktrunk#3800](max-sixty/worktrunk#3800) | | [31505266914](https://github.com/max-sixty/worktrunk/actions/runs/31505266914) | `tend-review` on [#3791](max-sixty/worktrunk#3791) | `claude -p` exit 1 | **no** | The quota exhaustion itself isn't a tend defect. What the second run exposed is: `Report failure` ran, hit a GitHub 502, and **exited 1 instead of degrading**, so the row was never appended. #3800 still reads `updated_at: 2026-08-11T15:00:53Z` with zero comments and a single row — it reports one stranded run when there were two, and the run it omits is the one whose review of #3791's new HEAD never happened. <details><summary>Log evidence for the attribution</summary> From run 31505266914's `Report failure` step (13.8 s, ending in failure): ``` 2026-08-11T15:09:23.2761440Z non-200 OK status code: 502 Bad Gateway body: "<!DOCTYPE html>... 2026-08-11T15:09:23.3052745Z ##[error]Process completed with exit code 1. 2026-08-11T15:09:23.3059103Z ##[end-action id=__max-sixty_tend.__run_13;outcome=failure;conclusion=failure;duration_ms=13816] ``` The three earlier `gh` calls in the script are each ruled out, which leaves the append: - `run_issue_ensure_label` is `2>/dev/null || true`, so it can neither emit that stderr nor abort. - `run_issue_canonical` is read through `if ! EXISTING=$(...)`, whose failure path prints `::warning::Could not read this repo's tend-outage issues...` and exits 0. No `::warning::` appears anywhere in the run log, so the read succeeded. - `$EXISTING` was therefore #3800, and control reached the bare `gh issue comment` — the only unguarded write left. </details> ## Root cause `report-failure.sh` guards its read and leaves its append bare: ```bash if ! EXISTING=$(run_issue_canonical "$LABEL" open "$TITLE"); then echo "::warning::Could not read this repo's ${LABEL} issues, ..." exit 0 fi if [ -n "$EXISTING" ]; then printf '%s\n' "$ROW" | gh issue comment "$EXISTING" -F - # <- aborts under `set -e` ``` `rate-limit-preflight.sh`, the sibling caller of the same `lib/run-issue.sh`, already guards the identical call — added in `e5f0f9b`, whose comment reasons about exactly this: > Left bare it would abort here under `set -e`, costing the run the annotation below, which is worth more than the row: the issue already exists, so the annotation can still name what to close, while the row is one line of evidence among the rows the other refusals appended. That argument transfers verbatim; `report-failure.sh` was simply never given the same treatment. This is the common write path, not a corner: once a tracker is open, every later failure in the same incident appends through it — a previous outage cluster put 8 rows on [worktrunk#3780](max-sixty/worktrunk#3780) this way, all through this one call. ## The fix Wrap the append, warn, let the step end clean. Deliberately **not** symmetric — the create branch keeps its abort, because `test_report_failure_propagates_a_failed_create` already fixes that policy and the reasoning still holds: with no tracker open, a failed create leaves no record of the outage anywhere, so reddening the step is the only surviving signal. An append has a tracker that already carries the incident. The new test's docstring names the asymmetry so it doesn't get "tidied" later. `test_report_failure_survives_a_failed_append_to_the_open_tracker` reproduces the production failure: without the change it fails with `returncode 1` and the row dropped; with it, exit 0 plus the warning. Full file passes (41 tests). ## Gate assessment - **Gate 1 — confidence: High, acted on.** One production occurrence this window, but not a fresh judgement call: `e5f0f9b` is the project's already-accepted ruling that this exact mechanism on this exact call is a defect, applied to one of the two call sites. The remaining site has now fired. Failure is **structural** — given a 5xx on the append, the abort is deterministic, not a model behaviour that might go differently on a replay. - **Gate 2 — magnitude: targeted fix, normal bar.** One `if` wrapper plus a warning line, mirroring an existing guard byte-for-byte. It removes an inconsistency between two callers rather than introducing new policy. - **Dedup.** [#859](#859) (persistent failures appending *too many* rows) and [#809](#809) (matrix-leg dedup) both work the opposite axis — how many rows to write, not what happens when a write fails. [#857](#857) widens which step failures report at all. None touch the abort. No open PR modifies this file. ## Not addressed here The stranded `tend-review` on [worktrunk#3791](max-sixty/worktrunk#3791) has no retry path — the run failed before stamping the commit, and nothing re-fires until the next push, so that HEAD stays unreviewed. [#816](#816) raised both halves of this ("nothing re-runs the trigger it names… leaves the PR silently un-reviewed forever") and was closed COMPLETED on 2026-08-07; the naming half did ship, via the nightly enricher. On this evidence the re-run half looks still live, but that's one observation, so it goes in the evidence log to accumulate rather than reopening anything here. The two do compound, which is worth flagging: the tracker is the list a maintainer would re-run from, so a dropped row makes a stranded run correspondingly harder to find. That's the argument for this one-line guard, not for widening the PR. --------- Co-authored-by: tend-agent <270458913+tend-agent@users.noreply.github.com>
## Problem Gap 2 of #816: a `tend-outage` row names the trigger a dead session stranded, but nothing re-runs it. `tend-review` fires only on `pull_request_target`, so a PR whose one review attempt died stays unreviewed until someone happens to push again — and the outage issue that recorded it stays open, folding the next incident into a stale one. The recovery shape has been running in worktrunk's `running-tend` overlay since the incident that filed #816, across two outages of both flavours (a 5-hour session-limit exhaustion and a weekly-limit exhaustion). It is generic — every consumer running `tend-review` on `pull_request_target` loses reviews the same way — so worktrunk's maintainer asked for it upstream rather than kept per-repo ([max-sixty/worktrunk#3742](max-sixty/worktrunk#3742 (comment))). The companion PR removes it from worktrunk's overlay once this lands. ## Solution One section in the bundled `review-runs` skill, at the end of Step 1 where failed runs are already being classified. Four rules, each a recipe: - **Find** the open `tend-outage` issue and extract its run/trigger rows. Empty on most days, so the check is a cheap no-op. - **Diagnose before re-running.** The issue body says only "The bot failed to process a request". The annotation is the cheapest next look — and #818, if it lands, makes it name the cause on the most common path — but when it doesn't, the session log carries the `<synthetic>` message. Both subscription limits are listed, because they reset on different clocks: assuming the session window's reset understates a weekly exhaustion, which can strand most of a day. - **Re-run only what won't recover on its own.** Scheduled workflows come back on their next cron tick; only event-triggered runs (`review`, `mention`, `triage`, `ci-fix`) strand. Confirm the work is still missing first — a later push often re-triggers the workflow by itself. - **Order the re-run after a clean run**, not after an assumed clock. Re-running into a still-exhausted quota just refills the outage issue with fresh rows. Closing the drained issue is part of the recipe: `report-failure.sh` auto-closes only duplicates from the create-create race, never the surviving issue. ## Testing Documentation only — no generator or action code paths change, so there is nothing for `wt test` to exercise. The commands in the section are the ones worktrunk's sweep has been running: the `grep -oE 'runs/[0-9]+|\| #[0-9]+'` extraction matches the row format `report-failure.sh` emits (`| ${TIMESTAMP} | [workflow run](${RUN_URL}) | ${REF} |`), and the workflow-trigger split is read off the generator templates — `review.yaml.j2` is `pull_request_target`, `ci-fix.yaml.j2` is `workflow_run`, `mention.yaml.j2` is `issue_comment` plus the relayed dispatch, `triage` is `issues`, and `scheduled.yaml.j2`/`notifications.yaml.j2` are `schedule`. ## Scope Skill guidance only, and deliberately not the automated recovery pass #816 sketched — that carries the design tradeoffs #818 flagged (which workflows, the quota-window ordering, confirming the work is still missing). Those are exactly the judgements a skill can make per-case and a script cannot, so the agent-driven half is worth having whether or not a deterministic pass ever lands. Textually disjoint from the other open outage-path changes: #818 is in `claude/action.yaml`, #823 and #836 and #809 are in `shared/steps/report-failure.sh`. Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com>
…1243 # Conflicts: # generator/tests/test_shared_steps.py # shared/steps/report-failure.sh
tend-agent
left a comment
tend-agent
left a comment
There was a problem hiding this comment.
One finding, from the main merge in 6894b68 rather than from 55bb271 — the anchor fold is right, and run_issue_anchor is now the only place the [workflow run](…) string is built (grep -rn 'workflow run\](' over the tree returns just lib/run-issue.sh:61).
The merge brought main's failed-write policy onto the append — a 5xx on gh issue comment now warns and exit 0s instead of aborting — but the reconcile pipeline directly below it stayed bare. It is the last statement in the EXISTING branch, so under set -eo pipefail a failure on gh api --paginate --slurp takes the whole script's exit status with it, after the row was successfully posted. The result is a red Report failure step carrying no annotation to say why, on precisely the job whose failure someone is about to diagnose — and the append immediately above it went to some length to avoid exactly that.
The deletes inside the loop already carry || true, and both neighbouring fallible operations (the run_issue_canonical read at the top, the append) warn and continue, so the reconcile's read is the one unguarded operation left in the block. Suggestion inline.
How the abort was confirmed
Against this HEAD, with a fake gh that 502s only on …/comments?per_page=100 and answers everything else normally (open tracker #8, guard sees no anchor, so the append path runs and posts the row):
$ bash shared/steps/report-failure.sh
gh: 502 server error
EXIT=1
No ::warning::, and the row was posted — the step reddens over a completed write. The [ -z "$DUP_ID" ] && continue inside the loop is fine, incidentally: a non-last component of an AND-list is exempt from errexit, and the loop exits 0 on the normal path.
With || echo "::warning::…" appended to done, the same run exits 0 and emits the annotation. || binds to the whole pipeline, so no re-indentation is needed.
Not flagged: test_report_failure_survives_a_failed_append_to_the_open_tracker doesn't discriminate the merge's new exit 0 — without it the leg would fall through to a reconcile that finds nothing of its own to delete, so the observable behaviour is the same. It's an optimisation, not a correctness fix, and not worth a test.
…ails The comment reconcile is the last statement in the append branch, so under `set -eo pipefail` a 5xx on `gh api --paginate --slurp` took the script's exit status with it — after the row had already been posted. That reddens the `Report failure` step with no annotation naming why, on precisely the job someone is about to diagnose. Guard it the way the append immediately above and the tracker read at the top are guarded: warn and continue. A failed reconcile leaves duplicate rows on the tracker, which is the cheaper loss than a red step over a completed write. `||` binds to the whole pipeline, so `done ||` covers the read, the filter, and the delete loop.
tend-agent
left a comment
tend-agent
left a comment
There was a problem hiding this comment.
The reconcile guard in 3289113 closes the last finding, re-derived here rather than taken on the commit message's word. Reverting just the || echo "::warning::…" from done and re-running the suite fails exactly one test — test_report_failure_survives_a_failed_reconcile_read, returncode=1, stderr='gh: 502 server error' — and nothing else, which is the right discrimination: the other fourteen characterise behaviour the guard doesn't change. With it in place the same fake exits 0, emits the annotation, and COMMENT_BODIES still carries the row, so the write that already landed is not lost to the cleanup that failed after it.
|| binding to the whole pipeline rather than the while alone is what makes the one-line form correct, and the mutation run exercises that end to end — the failure is injected on gh api --paginate --slurp, the first component, and is still caught. That also leaves the deletes' inner || true doing its own job unchanged.
419 passed in generator/; test, lint, test-proxy, test-codex-surface, test-install-tend-scripts, test-worker all green on this commit. No further findings; not approving only because the PR is self-authored.
…857) ## Problem A run that fails **before** the agent step never reaches the outage tracker. `Report failure` is gated on the agent step specifically: ```yaml - name: Report failure if: failure() && steps.claude.outcome == 'failure' ``` Twelve steps run ahead of `claude` — the security preflight, the rate-limit preflight, bot-ID resolution, auth validation, proxy/uv install, sensitive-config restore, prompt composition, the mitmproxy cache, the sandbox build, the binary and plugin installs, and the adopter's `sandbox_setup:`. When any of them fails, `steps.claude.outcome` is `skipped`, the conjunct is false, and the run goes out red with no `tend-outage` issue and no comment on an existing one. The work it stranded leaves no trace anywhere a maintainer looks. `codex/action.yaml` carries the identical gate on `steps.codex.outcome`. ## What it cost On `max-sixty/tend` between 2026-08-05T11:37Z and 2026-08-06T00:07Z, the rate-limit preflight aborted **every** agent run on the repo — 36 failed runs across four workflows: | Workflow | Failed / total | |---|---| | `tend-notifications` | 19 / 20 | | `review-reviewers` | 12 / 13 | | `tend-review` | 4 / 4 | | `tend-mention` | 1 / 1 | Every one aborted on the same line, with the burst counters at zero — the bot's daily item count had crossed the spike threshold and, since the guard runs before anything that could change that count, it stayed crossed until the UTC date rolled: ``` Rate limit: burst=0 PRs, 0 issues (20min); today=16 (limit: 15) ##[error]Rate limit: bot created 16 items today, above spike limit of 15 (baseline: 17 over past 6 days) ``` And in all 36, the outage step never ran — from the raw logs of both a [`review-reviewers` leg](https://github.com/max-sixty/tend/actions/runs/31055440109) and a [`tend-review` run](https://github.com/max-sixty/tend/actions/runs/31047860817): ``` ##[start-action display=Report failure;id=__max-sixty_tend.__run_13] ##[end-action id=__max-sixty_tend.__run_13;outcome=skipped;conclusion=skipped;duration_ms=0] ``` Zero `tend-outage` issues were filed or commented on across the whole 12.5-hour window; the label's most recent issues are still #831 and #832 from 2026-08-04, both closed. Those two got filed precisely because that outage failed *inside* the agent step. So the tracker works — it just can't see the half of the action that runs first, which is where a whole-repo, day-long stop lives. The blackout self-cleared at 00:00Z when the daily counter reset, so nothing here needs a revert; the reason a maintainer never saw it is what this PR fixes. ## Fix Gate on the job being red rather than on which step reddened it, in both harness actions, and correct the `report-failure.sh` header comment that documented the old contract. A pre-agent failure strands exactly the same work as an agent failure, so it belongs in the same tracker. The widened gate also admits post-agent failures (`Mark event notification read`, `Token usage`). Those are rarer and the agent's work has already shipped by then, but the run is still red and still worth a row — an outage issue that occasionally over-reports is the right side to err on relative to one that misses a 12-hour stop. **One exclusion: the security preflight.** `security-preflight.sh` failing means the repo isn't safely gated for the bot — an unprotected default branch, or an update ruleset the bot can bypass. That's a config refusal, not an outage, and it's persistent: it stays failing until a human fixes the repo, so under a bare `failure()` gate it would file an issue titled "Bot temporarily unavailable" and append a row on every subsequent trigger, indefinitely, while the reporter records only a run link and so never names the cause. It's also the one path where reporting has the action write to the repo (`gh issue create`, plus `gh issue close` on the reconcile path) with the bot's PAT right after the security gate refused to let it operate there. The step now carries `id: security` and the gate is `if: failure() && steps.security.outcome != 'failure'`; it's the first step in both actions, so every other failure leaves that outcome `success` and the widening is otherwise unaffected. Note that the PAT-write argument is what carves it out, not persistence: `Validate auth configured` and the adopter's `sandbox_setup:` also fail deterministically until a human edits config, and they stay in. Bounding that repeated append belongs in `report-failure.sh`, where one change covers every such step without an enumerated exclusion list — tracked in #859. **The rate-limit abort's remediation is itself a counted item.** Worth stating because the direction is counterintuitive when this is read back later: the tiers `rate-limit-preflight.sh` enforces count bot-authored issues — `RECENT_ISSUES` via `repos/$REPO/issues?creator=$BOT`, `TODAY_POSTS` via `search/issues?q=author:...` — so the first abort under the new gate creates a `tend-outage` issue and thereby nudges the very counter it tripped on. It's self-limiting rather than a loop: every later failure appends a *comment* to the now-open issue, and comments appear in neither query, so the exposure is +1 item per open-issue cycle. This is diagnosability only — it doesn't touch the rate-limit thresholds. #856, from a sibling leg of this same run, retunes the guard that caused this particular blackout by demoting its spike tier to a creation pause. The two are complementary rather than overlapping: #856 stops the spike tier from failing the run at all, and this PR makes the tiers that still abort — the two burst checks and the hard limit it keeps — plus every other pre-agent step land in the tracker when they do. ## Gate assessment - **Evidence level**: Critical — 36 failed runs, four workflows, a 12.5-hour total stop of the bot on its own repo, invisible end to end. Acts on one occurrence. - **Structural, not stochastic**: no decision point. `steps.claude.outcome` is `skipped` for every pre-agent failure, so the condition is false 100% of the time, for every consumer of both actions. - **Change type**: targeted fix — one `if:` expression per action plus a comment correction. - **Passes both gates.** Evidence log: https://gist.github.com/dca23a6e6a0d8cae2665944ba31676fb Related but distinct — both assume the agent step failed and so never fire on this path: #818 (naming the cause in the exited-non-zero annotation) and #809 (deduping outage comments across matrix legs). --------- Co-authored-by: tend-agent <270458913+tend-agent@users.noreply.github.com>
… and stop the fetch limit re-truncating it (#838) `list-recent-runs.sh`'s dropped-tick recovery anchors the window floor on the previous **completed** run of the analyzing workflow. `completed` is a status, not a conclusion, so a run that failed before its agent produced any analysis advances the anchor exactly as if it had covered its hour. After an outage the next green run therefore resumes one tick back and reports an all-clear for a window nothing ever looked at — and the gap is unrecoverable, because the following run's floor advances past it too. This is not hypothetical: it just consumed ~15 hours of `review-reviewers` coverage. ## What happened Every `review-reviewers` run from [30897445507](https://github.com/max-sixty/tend/actions/runs/30897445507) (09:41Z) through [30959115609](https://github.com/max-sixty/tend/actions/runs/30959115609) (23:10Z) failed — 16 consecutive runs, all five matrix legs, `claude -p` exiting 1 after ~4s. The `result` event in every leg's session log is identical: `is_error: true`, `num_turns: 1`, `result: "You've hit your weekly limit · resets 12am (UTC)"`, with `token-usage.json` all zeros. The agent never started, so none of those runs analyzed anything. (The outage itself is already tracked by #831 and the annotation-legibility work in #816 / #818 — this PR is only about the window the outage swallowed.) The last run that did analyze anything is [30893072876](https://github.com/max-sixty/tend/actions/runs/30893072876) at 08:42Z, which is also the last entry in the `numbagg/numbagg` evidence gist. At this run's tick (23:47Z intended), the recovery query picked `{"conclusion":"failure","createdAt":"2026-08-04T23:10:57Z","databaseId":30959115609}` and floored the window at 22:47Z, so `list-recent-runs.sh` returned exactly one row: a `tend-notifications` pre-check no-op. Everything `numbagg-bot` actually did in the swallowed window sat outside that floor — PRs [#719](numbagg/numbagg#719) and [#720](numbagg/numbagg#720), issue [#721](numbagg/numbagg#721), three reviews and four inline comments on #719, and comments on #716, #721 and #1. That is the densest window numbagg has had in days, and per the skill's Step 1 ("If empty, record the run as all-clear … then skip to Step 6") it would have been recorded as a quiet hour. I only found it by diffing the run list against the gist's last recorded boundary by hand. ## The change Anchor on the previous **successful** run rather than any completed one, filtering server-side with `--status success` (the flag takes conclusions as well as statuses), and when the existing 6h cap clamps the recovered floor, say so on stderr so the caller records a coverage gap instead of a false all-clear. A partially-failed matrix run counts as a failure here, which only ever widens the window — overlap is re-offered work the caller dedups against its own evidence log, whereas a gap is silently unanalyzed. Widening the window then exposed a second truncation one layer down, so the fetch loop moves with it. It passed `--limit 50` while a recovered window spans up to 8h, and `gh run list` returns newest-first — so a workflow over the limit silently drops its *oldest* runs, which are exactly the ones in the gap the anchor just reached back for. On `max-sixty/tend`, `tend-mention` alone produces 57 runs in an 8h window. The limit is now 200, and a workflow returning exactly the limit warns rather than truncating in silence. Two smaller pieces: the anchor query routes through `gh_retry` and exits non-zero instead of `2>/dev/null || true`, so a transient API error can no longer masquerade as "no successful run" and emit a confident warning naming a cause that didn't happen; and `review-reviewers/SKILL.md`, which still said "if empty, record all-clear" with nothing about the stderr warnings, now treats any `WARNING:` as a coverage gap — without that, an agent following the skill literally would print the warning and record an all-clear anyway. When every tick fires and succeeds the anchor is the previous tick, `prev_intended == intended - 3600 == COMPLETED_AFTER`, and the comparison is a no-op — output is byte-identical to today's on the healthy path. <details><summary>Verification against live data</summary> Same tick, same repo, before vs. after: ``` # anchor query — before (--limit 10, no conclusion filter) {"conclusion":"failure","createdAt":"2026-08-04T23:10:57Z","databaseId":30959115609} # anchor query — after (--limit 50, conclusion == "success") {"conclusion":"success","createdAt":"2026-08-04T08:42:12Z","databaseId":30893072876} ``` `TARGET_REPO=numbagg/numbagg ./list-recent-runs.sh` after the change returns 5 runs instead of 1, preceded by: ``` WARNING: the last successful 'review-reviewers' run started 2026-08-04T08:42:12Z, more than 6h back. Window floored at 2026-08-04T17:47:00Z; runs that completed before it are NOT in this list. Record a coverage gap, not an all-clear. ``` The recovered anchor (08:42:12Z) matches the gist's last recorded coverage boundary exactly. Also exercised: the no-successful-run branch (warns, floors at the cap, exits 0) and the unset-`GITHUB_WORKFLOW` branch (recovery skipped, unchanged). `shellcheck` clean; `bash -n` clean. Also exercised after the review follow-ups: the fetch-limit truncation warning (via a copy with `RUN_LIMIT=5`) and the anchor-query failure path (exits 1 rather than degrading). See [the follow-up comment](#838 (comment)) for the full branch table. </details> ## Gates - **Evidence level**: Critical — a clearly wrong outcome (the analysis records an all-clear for a window it never examined, and the window is then unreachable), which acts on 1 occurrence. Occurrences: 1 this run, 0 historical (no prior gist entry records an outage of a length that would trigger it). - **Structural, not stochastic**: no decision point. The anchor query is deterministic; replayed ten times it picks the failed run ten times. - **Change type**: targeted fix — one predicate, one query limit, two stderr warnings. Normal evidence bar under Gate 2, met. - **Dedup**: checked open issues #831, #830, #829, #828, #827, #824, #822, #817, #816, #801, #799, #750, #624 and open PRs #826, #825, #823, #821, #819, #818, #809. #816 / #818 / #823 / #809 all concern outage *reporting* — naming the cause in the annotation, the stranded trigger, per-run comment dedup — none touch window recovery. Searching issues and PRs for `list-recent-runs` surfaces only the three merged predecessors #526, #753 and #784. Evidence log: https://gist.github.com/19b5ab297bb7ac7e1e9a44d595ccde0f --------- Co-authored-by: tend-agent <270458913+tend-agent@users.noreply.github.com>
2 participants
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