Skip to content

fix(review-runs): inherit errexit so a failed gh call can't shrink the corrections window - #1124

Merged
max-sixty merged 1 commit into
mainfrom
fix/corrections-inherit-errexit
Sep 1, 2026
Merged

fix(review-runs): inherit errexit so a failed gh call can't shrink the corrections window#1124
max-sixty merged 1 commit into
mainfrom
fix/corrections-inherit-errexit

Conversation

@tend-agent

Copy link
Copy Markdown
Collaborator

review-runs-corrections.sh collects the maintainer-correction signals that Step 4 of review-runs turns into "no maintainer corrections" in the tracking issue — which later runs read as ground truth under Gate 1. Two of its three collections loop gh calls inside a command substitution, and bash does not apply set -e inside one unless inherit_errexit is set. A gh failure in any but the final iteration was therefore dropped from the JSON while the script still exited 0, reporting the surviving iterations as the whole window.

The fix is shopt -s inherit_errexit next to the existing set -euo pipefail, so every command substitution in the script aborts on a failed call rather than each collection needing its own guard.

Two regression tests, both failing before the change and passing after:

  • a 502 on the issues/comments endpoint (the first of the two the loop reads) — previously exited 0 with only the pulls/comments half present
  • a 502 on the first of two candidate PRs' reviews — previously exited 0 with only the second PR's reviews present
Why the existing guard didn't cover this

The script already reasons about set -e and this loop. Its comment above CANDIDATES explains that the candidate list is pulled into its own assignment because a failed --search inline in the for word list is invisible to set -e, and test_a_failed_candidate_search_aborts_rather_than_reporting_no_reviews pins that. That guard is correct and unchanged — but it covers the word list, not the loop bodies, which are the calls this PR fixes.

The distinction is inherit_errexit, not pipefail. pipefail does propagate the loop's status out of the pipeline, so a failure on the last iteration was already caught; only earlier iterations were silently dropped. The observed asymmetry:

$ bash -c 'set -euo pipefail; O=$(for i in 1 2 3; do
    if [ "$i" = 2 ]; then cat /nope 2>/dev/null; else echo "$i"; fi
  done | tr "\n" ","); echo "REACHED [$O]"'
REACHED [1,3,]        # exit 0

$ bash -c 'set -euo pipefail; shopt -s inherit_errexit; ...same...'
                      # exit 1

The pre-fix test output shows the real shape — the script emitted a complete-looking payload ("reviews": [], comments missing one endpoint) on stdout with 502 Bad Gateway on stderr and status 0.

review-runs-corrections.sh is the only script in the repo with this shape; the other nine under plugins/tend-ci-runner/scripts/ have no multi-command substitution, so the change is scoped to this file.

Verified with uv run pytest (884 passed).

max-sixty pushed a commit that referenced this pull request Sep 1, 2026
…1126)

`pre-commit` is not on the CI sandbox's PATH, but CLAUDE.md's Commands
block names bare `pre-commit run --all-files` as the lint gate — so a
tend session that follows it gets `command not found`. Three sessions
hit that in the last 24 h; two recovered with `uv tool run pre-commit`,
one substituted ruff plus shellcheck and ran a narrower gate than the
repo's. The Commands block now carries `uv tool run pre-commit run
--all-files` — the one form that works in both a human checkout and the
sandbox — and the paragraph below it says why the prefix is there and
what a narrower substitute costs: ten of the thirteen hooks, including
the three `repo: local` guards. `.claude/skills/release/SKILL.md`'s lint
step takes the same prefix.

<details><summary>Evidence, verification, and the alternative I didn't
take</summary>

**Verified live from inside the sandbox** (this run, as `tend-sandbox`):
`command -v pre-commit` is empty; `command -v uv` is
`/opt/hostedtoolcache/uv/0.12.8/x86_64/uv`; `uv tool run pre-commit
--version` prints `pre-commit 4.6.2`.

**Occurrences in the window** `2026-08-31T07:50:43Z →
2026-09-01T07:49Z`, from the session JSONL of each run:

| Run | Workflow | What happened |
|---|---|---|
|
[33423814339](https://github.com/max-sixty/tend/actions/runs/33423814339)
| `tend-mention` (#1121) | `pre-commit run --all-files` →
`/usr/bin/bash: line 1: pre-commit: command not found`, then retried as
`uv tool run pre-commit` |
|
[33425029498](https://github.com/max-sixty/tend/actions/runs/33425029498)
| `tend-mention` (#1121) | same `command not found`, same recovery; its
summary notes "via `uv tool run` — `pre-commit` isn't on PATH in this
sandbox" |
|
[33479300709](https://github.com/max-sixty/tend/actions/runs/33479300709)
| `tend-review` (#1124) | same `command not found`, **no retry** —
substituted `uv run ruff check` + `ruff format --check` and a standalone
shellcheck, so `typos`, `actionlint` and `uv-lock` went unrun |

Three further sessions
([33424356768](https://github.com/max-sixty/tend/actions/runs/33424356768),
[33440961515](https://github.com/max-sixty/tend/actions/runs/33440961515),
[33442043938](https://github.com/max-sixty/tend/actions/runs/33442043938))
plus `tend-nightly`
[33478582882](https://github.com/max-sixty/tend/actions/runs/33478582882)
went straight to `uv tool run pre-commit` without trying the bare form —
the workaround is already in circulation, just not written down. The
overlay's own weekly recipe already uses `uv tool run pre-commit
autoupdate` at `.claude/skills/running-tend/SKILL.md:196`.

**Classification.** Structural — the same conditions produce the same
failure every session. Cost class: **waste** this window; no wrong
outward action occurred. Run 33479300709 did not claim `pre-commit`
passed (its review said "shellcheck clean on the script", which is
accurate), so nothing false was posted. The remedy is one paragraph in a
file that already exists, which is why it clears Gate 3 despite the
waste class.

**The alternative, and why not.** `sandbox_setup: ["uv tool install
pre-commit"]` in `.config/tend.yaml` would put the binary on the sandbox
PATH, which is the shape
[#691](#691) was steered toward
("should we install into our environment in the workflow, rather than
having the agent do it?"). It's repo-level, so it would run on every
agent boot — including the ~11 `tend-notifications` sessions a day that
never lint — to spare a four-word prefix on the few that do. That trade
reads the wrong way against "Simplicity outranks efficiency", so I've
left the knob to you rather than taking it.

**Verification of this change:** `uv tool run pre-commit run --files
CLAUDE.md` passes (`typos`, `ruff format`, the plugin-skill and
install-tend sync hooks all green); `uv run pytest` is 882 passed.

</details>

---------

Co-authored-by: tend-agent <270458913+tend-agent@users.noreply.github.com>
@max-sixty
max-sixty merged commit d4936e5 into main Sep 1, 2026
7 checks passed
@max-sixty
max-sixty deleted the fix/corrections-inherit-errexit branch September 1, 2026 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants