skills(review-runs): drain triggers stranded by a failed session - #851
skills(review-runs): drain triggers stranded by a failed session#851worktrunk-bot wants to merge 1 commit into
Conversation
Removes the Outage Recovery section from the overlay; the guidance now lives in tend's bundled `review-runs` skill, proposed in [max-sixty/tend#851](max-sixty/tend#851). **Why upstream.** Nothing in the section was worktrunk-specific: the `tend-outage` issue shape, the `pull_request_target`-only trigger that makes a dead `tend-review` unrecoverable, the session-log message that names the quota limit, and the scheduled-vs-event-triggered split are all properties of tend itself. Every consumer running `tend-review` loses reviews the same way. That was already the disposition in [max-sixty/tend#816](max-sixty/tend#816) (Gap 2), which the upstream PR closes out. **What changed from the original PR.** This branch first carried a three-line correction naming the second subscription limit. That correction is folded into the upstream section instead, so the net diff here is the removal. **Sequencing.** Merging this before #851 lands leaves a window with no drain guidance in either place. The window costs one daily sweep at most, and the outage issue accumulates rather than losing data, so it's a maintainer call whether to hold this until upstream merges. --------- Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com>
## Problem The spike rate limit is a circuit breaker with no reset: once tripped, the only cure is the UTC rollover. A legitimate nightly sweep — six PRs filed between 00:20 and 00:24 — pushed the day's count to 16 against a limit of 15, and every `tend-review` run from 11:47 UTC onward died at the preflight. Six hours with no reviews, and no signal anywhere a person would look: a failed preflight writes an annotation onto a job nobody opens. #851's own review was one of the ones stranded. Two separable gaps: no **notice** that the bot had stopped, and no **reset** short of waiting for midnight. ## Solution On a spike trip the preflight files or reopens a `tend-rate-limit` issue listing the runs it refused. Opening it is the notice. Closing it is the approval, and doubles the ceiling for the rest of the UTC day; each further close doubles again, so the breaker re-arms after use rather than switching off. A doubling rather than a flat increment because the ceiling it lifts is itself proportional — `10 + 2 * daily_avg` is 15 here but 40 on a repo filing 15 a day, where a flat bump would mean closing the issue all afternoon. At the formula's floor the two coincide (10 doubles to 20), so they diverge only where a flat bump stops working. The burst limit stays non-resumable: ten PRs in twenty minutes is a loop, not a busy day, and there is nothing there to wave through. `ABORT` is set true at three points and never cleared, so an approval cannot release a burst trip — and a burst trip files no issue at all, since one offering to lift the ceiling would promise a recovery that closing it cannot deliver. **Approval is a check, not an instruction the bot could be talked out of.** The preflight counts only closes whose actor is neither the bot nor a GitHub App. GitHub admits only an issue's author or a triage/write collaborator to close it, and the bot is the author, so what remains is exactly the maintainers — no allowlist to keep. Three things pin that down, because the bot holds `issues: write` and could otherwise arrange an approval for itself: - **The anchor is an issue the preflight filed** — lowest-numbered, authored by the bot, under the pause title. A label alone pins neither author nor title, so without this, labelling any issue would nominate it. - **An approval must postdate the label going on.** Moving the label onto an issue closed earlier would otherwise import that close as an approval. On a real pause issue the label is applied at creation, so nothing genuine is excluded. - **The actor is matched by numeric id, and Apps are excluded by type.** A rename doesn't shake the first; `github-actions[bot]`, which a workflow's own `GITHUB_TOKEN` acts as, is caught by the second. Identity comes from the credential rather than from config. The preflight runs as the bot, so `gh api user` yields the login the counts need and the id the approval filter compares. The configured name fed both before, and every way it could be wrong failed open: stale after a rename, the counting queries match nothing and the limit never trips at all; empty, the approval filter reduces to "login is not the empty string" and the bot's own close approves it. The preflight now publishes both facts for the rest of the job, replacing the `Resolve bot ID` step that re-derived the id from the configured name. <details> <summary>Why the bookkeeping labels moved into the generator</summary> The rate-limit issue collects one comment per refused run, and `tend-mention` skipped only `tend-outage` — so each row would have re-triggered mention, whose handle job trips the same limit and appends another row. That is the self-sustaining loop the existing guard exists to stop, reachable again through a label it did not know about. `BOOKKEEPING_LABELS` in `workflows.py` behind a `not_bookkeeping()` macro gives triage and mention one list, so the next such label reaches both at once. Rendered as `contains(…) == false`, since a value opening with `!` is a tag indicator to YAML and only parses inside a block scalar. The worker keeps its own copy for now — unifying it needs a story for issues already filed without the label. </details> ## Testing Ten cases in `test_shared_steps.py` cover the ceiling arithmetic, day-scoping, both actor exclusions, the rename case, the refusal when identity can't be read, and reopen-rather-than-duplicate. The fake `gh` runs the script's own `--jq` expression against a fixture with real jq, because that filter *is* the behaviour under test — a fake that pre-filtered would assert nothing. Verified by mutation rather than by passing. Each of these reddens exactly the case named and nothing else: deleting the two actor conditions (bot, rename, App), putting the filter back on login (rename), dropping the label-ordering rule (relabel), dropping the title filter or the `--author` scope (foreign anchor), reverting the shared predicate to label-only (foreign anchor and reconciler), and removing the day floor (yesterday's approval). `run_issue_matching` is the single definition of which issues belong to a record — label, author, title. The anchor lookup and the duplicate reconciler are the same call, so they cannot come to disagree about which issue is real. **Not yet exercised against real GitHub.** No run has filed a pause issue, reopened one, or been approved end to end. `tend-agent/tend-integration` exists for that and would be worth a pass before this is relied on. ## Deferred `TODO.md` records the automatic re-runner for work a trip refused, blocked on confirming tend's re-runs work at all — building one over a broken re-run path would move the symptom from "my review never came back" to "the recovery workflow ran and my review still never came back". Recovery meanwhile is one `gh run rerun <id> --failed` per row in the issue's table. Also open: an approval cannot be revoked, since reopening doesn't remove the close event. Counting only closes since the most recent reopen by a person would give a "stop" that works inside the day; left out here because it adds complexity to a security-critical script. > _This was written by Claude Code on behalf of max-sixty_ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Field evidence from The drain is keyed on an open Concretely, as of 2026-08-07T02:44Z, four
All four were created during the critical Actions incident of 2026-08-06T15:22:49Z, which resolved at 02:04:44Z — they are still parked 40 minutes past resolution. It isn't a backlog: sibling The three scheduled ones lose nothing — the next cron tick picks up the same unread queue. The One occurrence, so I'm not proposing a change — recording it in case widening the predicate from "open outage issue" to "open outage issue, plus runs still non-terminal well past their creation" is cheap before this lands. Full evidence log: https://gist.github.com/192514ea2c36586f9b7f842a482d62ab |
|
Field evidence from This PR's body says "The companion PR removes it from worktrunk's overlay once this lands." max-sixty/worktrunk#3742 merged at 2026-08-05T16:57:13Z — 38 minutes after this PR was opened — and deleted the whole Tonight it produced exactly the failure this PR predicts, on the first outage since the removal. A Claude session-limit outage ( Seven of those rows are Two smaller notes for the recipe, both consistent with what's already written here:
No new PR from this run — this one already carries the fix. Sources
|
2 participants
Problem
Gap 2 of #816: a
tend-outagerow names the trigger a dead session stranded, but nothing re-runs it.tend-reviewfires only onpull_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-tendoverlay 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 runningtend-reviewonpull_request_targetloses reviews the same way — so worktrunk's maintainer asked for it upstream rather than kept per-repo (max-sixty/worktrunk#3742). The companion PR removes it from worktrunk's overlay once this lands.Solution
One section in the bundled
review-runsskill, at the end of Step 1 where failed runs are already being classified. Four rules, each a recipe:tend-outageissue and extract its run/trigger rows. Empty on most days, so the check is a cheap no-op.<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.review,mention,triage,ci-fix) strand. Confirm the work is still missing first — a later push often re-triggers the workflow by itself.Closing the drained issue is part of the recipe:
report-failure.shauto-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 testto exercise. The commands in the section are the ones worktrunk's sweep has been running: thegrep -oE 'runs/[0-9]+|\| #[0-9]+'extraction matches the row formatreport-failure.shemits (| ${TIMESTAMP} | [workflow run](${RUN_URL}) | ${REF} |), and the workflow-trigger split is read off the generator templates —review.yaml.j2ispull_request_target,ci-fix.yaml.j2isworkflow_run,mention.yaml.j2isissue_commentplus the relayed dispatch,triageisissues, andscheduled.yaml.j2/notifications.yaml.j2areschedule.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 inshared/steps/report-failure.sh.