fix(actions): report an outage when the run fails ahead of the agent - #857
fix(actions): report an outage when the run fails ahead of the agent#857tend-agent wants to merge 3 commits into
Conversation
Report failure was gated on steps.claude/steps.codex outcome, so the twelve steps that run before the agent — both preflights, auth validation, the sandbox build, the adopter's sandbox_setup: — failed the job without filing or commenting on a tend-outage issue. A rate-limit preflight abort held every workflow on max-sixty/tend for 12.5 hours across 36 runs with no record anywhere. Gate on failure() instead, and correct the report-failure.sh header that documented the old contract.
tend-agent
left a comment
There was a problem hiding this comment.
Two things worth a second look before this lands. The core change is right — steps.claude.outcome == 'failure' is false for every pre-agent abort, so the tracker structurally can't see them.
The security preflight is a config refusal, not an outage. security-preflight.sh is the one pre-agent step whose failure means "this repo is not safely gated for the bot" — unprotected default branch, or an update ruleset the bot can bypass. Under if: failure() that repo now files a tend-outage issue and appends a row on every subsequent trigger, indefinitely: the failure is persistent, not transient, so the issue titled "Bot temporarily unavailable" with "Close it once the outage is resolved" never resolves, and since the reporter records only a run link the actual cause is never named in it. It's also the one case where the widened gate has the action write to the repo (gh issue create, and gh issue close on the reconcile path) with the bot's PAT after the security gate refused to let it operate there. Excluding just that step keeps everything else the PR is after — add id: security to the preflight step and gate if: failure() && steps.security.outcome != 'failure'. Every other pre-agent failure leaves that outcome success, so the widening is unaffected. Flagging as a judgment call rather than a defect.
The rate-limit abort's remediation is itself a counted item. The tiers rate-limit-preflight.sh keeps count bot-authored issues — RECENT_ISSUES via repos/$REPO/issues?creator=$BOT, and TODAY_POSTS via search/issues?q=author:.... The first abort under the new gate creates a tend-outage issue, so the guard's response to tripping nudges the counter it tripped on. It's self-limiting — 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, not a loop. Worth a line in the PR body rather than a code change, I think, since the direction is counterintuitive when read back later.
One inline suggestion on the codex comment.
A security-preflight failure is a persistent config refusal, not a transient outage: it stays failing until a human fixes branch protection, so under a bare `failure()` gate it would file "Bot temporarily unavailable" and append a row on every later trigger without ever naming the cause. It is also the one path where reporting would have the bot write to the repo with its PAT right after the security gate refused to let it operate there. Also correct the codex comment: that action has no `sandbox_setup` input, so the adopter's setup runs in the caller workflow before the composite action starts.
|
Both taken. Pushed adbb6dd. Security preflight — excluded. Agreed, and the second half of the argument is what settled it: not just that "Bot temporarily unavailable" never resolves against a persistent config refusal, but that reporting is the action writing to the repo with the bot's PAT immediately after the security gate refused to let it operate there. Implemented as suggested — Rate-limit counter — noted in the body, no code change. Agreed on the disposition. Verified the mechanism before writing it up: One correction to the PR body while I was in there: the widened-gate comment in |
tend-agent
left a comment
There was a problem hiding this comment.
One follow-on from the exclusion; the rest of the widening reads right to me.
The security preflight isn't the only pre-agent step whose failure is persistent. The argument that carved it out — an issue titled "Bot temporarily unavailable" collecting a row on every trigger, because the failure won't clear until a human changes the repo — applies unchanged to two other steps the widened gate now admits:
Validate auth configuredfails whenever the harness credential input is empty.security-preflight.shauthenticates withGITHUB_TOKEN, not the model credential, so it passes and the new exclusion doesn't fire. This is what a per-workflowharness: codextrial withoutOPENAI_API_KEYlooks like, or a credential-secret rename — the rest of the repo keeps working while that one workflow reddens on every trigger.Run adopter sandbox setup(Claude action) fails on any error in the adopter'ssandbox_setup:— a bad package name, a moved URL. Deterministic, and red on every run until the adopter edits their config.
Either one then appends a row per trigger to an issue that records only a run link, so an adopter reading it sees "The bot failed to process a request" with no mention of a missing secret. The nightly enricher names the cause within a day, which bounds the confusion — but that's equally true of the security preflight, and it was excluded anyway. So what actually separates the excluded step from these two is the PAT-write argument on its own, not the persistence one.
I'm not suggesting more id:s — an enumerated exclusion list gets fragile as steps move. The narrower shape is to bound the append itself: skip commenting when the open outage issue already carries a row for this workflow inside some window, which covers every persistent-failure class at once. #809 is adjacent but doesn't reach it — it keys on the run id, so it collapses a matrix's legs within one run and still appends once per trigger across runs.
Reasonable to land the widening as-is and take that separately; flagging so the choice is deliberate rather than inherited from the one step that got a carve-out.
…tionale The persistence argument doesn't single out the security preflight — `Validate auth configured` and the adopter's `sandbox_setup:` fail the same deterministic way and stay in the tracker. What actually carves it out is the PAT-write argument alone, so say that and point the repeated- append problem at #859, where a fix in report-failure.sh covers every such step without an enumerated exclusion list.
|
The critique lands, and I've made the choice deliberate rather than leaving it inherited. Pushed 3bf1d06. The persistence argument doesn't single out the security preflight. Verified both counterexamples before agreeing. I've rewritten the comment to say exactly that rather than lead with persistence, since the old wording implied a distinction that doesn't hold and would have justified the next carve-out by analogy. No more Landing the widening as-is, with #859 as the follow-up. |
) ## Problem `review-reviewers` matrixes over five target repos ([`review-reviewers.yaml`](https://github.com/max-sixty/tend/blob/f3e309a475af03147ce6290104c85baafe6ad9f5/.github/workflows/review-reviewers.yaml)), and all five legs of a tick share one `$GITHUB_RUN_ID`. Step 5 of the skill hands every leg the same branch recipe: > **PR** (default): Branch `hourly/review-$GITHUB_RUN_ID`, fix, commit, push, … So the moment two legs of the same tick both find something worth a PR, they contend for one ref. The loser's `git push` is rejected as a non-fast-forward and it has to recover mid-session; the ordering that hurts more is the other one, where the second leg pushes *before* the first opens its PR and the two unrelated concerns land on a single branch — the atomic-PR rule broken by the branch name, not by any judgement the agent made. This just happened on run 31058673934: the `max-sixty/cargo-affected` leg's push to `hourly/review-31058673934` was rejected against a sibling leg's commit, which by then was #856. It recovered by suffixing the target name (#857), and an earlier tick shows the same improvisation baked into `hourly/review-30962483562-numbagg` — legs have been working around the recipe rather than following it. The run ID is non-unique along a second axis too: the same section caps a leg at two PRs (`**Limit to at most 2 PRs per run**`), and a leg that acts on two findings computes one name for both — same collision, one job instead of two. ## Fix Put both discriminators in the recipe: `hourly/review-$GITHUB_RUN_ID-<target-repo-name>-<topic-slug>`. The target name is unique across the matrix by construction, the topic slug separates a single leg's two PRs, and the branch stays greppable back to its run. Scoped to this skill. `review-runs` carries the same `$GITHUB_RUN_ID`-only shape, but `tend-review-runs.yaml` has no matrix, so a run there is a single job and the name can't collide — no change needed and none made. **Overlaps #845 on this line.** That PR rewrites the same recipe to rename the prefix (`hourly/` → `review-reviewers/`) as part of its cadence change, and keeps `$GITHUB_RUN_ID` as the whole discriminator — so it ships the collision under a new name. The two are orthogonal in substance and conflict only textually: whichever lands second wants `<prefix>/review-$GITHUB_RUN_ID-<target-repo-name>-<topic-slug>`, taking the prefix from #845 and the suffixes from here. ## Gate assessment - **Evidence level**: High — the collision is deterministic given two acting legs, and there are two independent traces of it (this run's rejected push, and a prior tick's hand-suffixed branch). The one-leg-two-PRs axis is structural rather than traced. - **Structural, not stochastic**: no decision point. Every leg is told to compute the same name from the same variable; replayed ten times it collides ten times. - **Change type**: targeted fix — one line of the recipe. - **Passes both gates.** Evidence log: https://gist.github.com/dca23a6e6a0d8cae2665944ba31676fb --------- 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>
Problem
A run that fails before the agent step never reaches the outage tracker.
Report failureis gated on the agent step specifically: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'ssandbox_setup:. When any of them fails,steps.claude.outcomeisskipped, the conjunct is false, and the run goes out red with notend-outageissue and no comment on an existing one. The work it stranded leaves no trace anywhere a maintainer looks.codex/action.yamlcarries the identical gate onsteps.codex.outcome.What it cost
On
max-sixty/tendbetween 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:tend-notificationsreview-reviewerstend-reviewtend-mentionEvery 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:
And in all 36, the outage step never ran — from the raw logs of both a
review-reviewersleg and atend-reviewrun:Zero
tend-outageissues 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.shheader 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.shfailing 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 barefailure()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, plusgh issue closeon the reconcile path) with the bot's PAT right after the security gate refused to let it operate there. The step now carriesid: securityand the gate isif: failure() && steps.security.outcome != 'failure'; it's the first step in both actions, so every other failure leaves that outcomesuccessand the widening is otherwise unaffected. Note that the PAT-write argument is what carves it out, not persistence:Validate auth configuredand the adopter'ssandbox_setup:also fail deterministically until a human edits config, and they stay in. Bounding that repeated append belongs inreport-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.shenforces count bot-authored issues —RECENT_ISSUESviarepos/$REPO/issues?creator=$BOT,TODAY_POSTSviasearch/issues?q=author:...— so the first abort under the new gate creates atend-outageissue 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
steps.claude.outcomeisskippedfor every pre-agent failure, so the condition is false 100% of the time, for every consumer of both actions.if:expression per action plus a comment correction.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).