Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ jobs:
tests/auto_rebase_health.bats \
tests/model_pricing.bats \
tests/test_review_cycle.bats \
tests/test_pr_automation_budget.bats \
tests/test_pr_runaway_regression.bats \
tests/test_ci_status.bats \
tests/test_push_protection.bats \
tests/test_verify_auth_scopes.bats \
Expand Down
137 changes: 137 additions & 0 deletions docs/postmortems/2026-06-pr-860-runaway.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
# Post-mortem: PR #860 runaway — agents self-triggering (June 2026)

**Status:** resolved (P0 fixes tracked in #926)
**Incident PR:** #860 (closed; re-opened from a fresh rebased branch)
**Provenance:** originally written in PR #924; that PR was closed in favor of
issue #926 as the single source of truth, and this doc is re-added alongside the code fixes.

---

## Summary

PR #860 — a routine `[Phase 2]` LSP change — ran away to **378 commits and 1,582
comments** over four days for a **net 18-file, +524/−982 diff**, and never
converged (`mergeable_state: blocked`). The entire volume was produced by **our
own agents looping**; there was no human in the thread. This is the same failure
class as #855 (comment storm) and #916 (trigger storm), on a new surface.

No human noticed for four days.

---

## Root cause — two interacting loops

### Loop A — self-mention comment storm (dominant; 93.6% of all comments)

The `pr-review` cascade posts an acknowledgement comment that **@-mentions the bot
itself**:

> `<!-- pr-review-agent mention-ack -->`
> `@donpetry-bot I'm on it — starting a fresh review now…`

That self-mention is an `issue_comment: created` event, so the mention listener
(`pr-review-mention.yml` → `pr-review-mention-reusable.yml`) interpreted it as a
review request and re-triggered the cascade → another identical ack. **1,481 of
1,582 comments were this one byte-identical comment, at a ~9s cadence for ~4.5h**
on 2026-06-24. The bot was talking to itself.

### Loop B — dev-lead autofix commit storm (the 378 commits)

Each `dev-lead-fix-*` push re-fired the cascade on `synchronize`; each review
re-triggered `dev-lead`. `[skip ci-relay]` suppressed the **CI** relay but **not**
the **review** re-trigger, and each fixup is a new SHA so same-SHA idempotency
(#899) didn't help. The autofix engine **re-appended** function definitions
instead of replacing them — **14 functions defined 2–4×**, one file bloated to
2,513 lines before a manual cut to 1,454. The loop was actively *corrupting* the
code, not converging.

### The one guard that fired was defeated

The #467 3-cycle cap escalated once — but its documented re-engage path was
*"mention the bot (`@donpetry-bot review`) for an immediate re-review,"* which is
exactly what the self-mention ack does. **A safety cap whose reset is reachable by
the runaway agent is not a cap.**

---

## Why every existing guard let it through

| Guard | Why it didn't stop #860 |
|---|---|
| `MAX_REVIEW_CYCLES=3` (`scripts/lib/review-cycle.sh`, #467) | Fired once, then re-armed by the bot's own self-mention; also reset on *machine* approvals |
| `MAX_FAIL_ATTEMPTS=2` (`scripts/dev-lead-fix-ci.sh`) | Counts engine *failures* only; succeeding fixups that push commits increment nothing |
| `auto-rebase-retry` `MAX_ATTEMPTS=3`, sweep `MAX_DISPATCH=20` | Per-run / per-tick, not per-PR-lifetime |
| `concurrency: cancel-in-progress` | Bounds parallelism, not sequential iteration count |
| same-SHA idempotency (#899) | Every fixup is a new SHA |
| `dev-lead-retry.yml` (every 2h) | **Amplifier** — re-ignites stalled loops |

Nothing bounded **total automated activity on one PR over its lifetime**, and
**no guard's reset required a human.**

---

## Learnings

1. **An agent must never be triggered by its own output.**
2. **A safety cap whose reset is reachable by the runaway is not a cap** — resets
must be human-gated.
3. **Bound the work item (the PR), not just each worker script.**
4. **Count actions, not just failures** — success can loop too.
5. **A new SHA is not new intent** — SHA-keyed idempotency can't break a fixup loop.
6. **Re-trigger crons amplify runaways** — they must check a stop condition before
re-dispatching.
7. **No human noticed for four days** — detection must be pushed, not pulled.

---

## Fixes

This work spans two repos.

### `petry-projects/.github-private` (this repo) — P0

- **Human-gated cap reset** (`scripts/lib/review-cycle.sh`): only a *human*
approval (a review with `state=APPROVED` by a non-bot author) or a human
escalation resets the cycle cap. The cascade's own approval marker and machine
approvals (`repair-pr-approvals`, bot approvals) no longer reset it. The
re-engage copy in `scripts/review-one-pr.sh` no longer offers "mention the bot"
as an automated escape hatch — only a human removing `needs-human-review`.
- **Per-PR automated-churn circuit breaker** (`scripts/lib/pr-automation-budget.sh`):
`compute_pr_automation_cycles` / `pr_budget_exhausted` count agent-authored
commits + review cycles + acks **since the last human interaction**; default
`MAX_PR_AUTOMATION_CYCLES=10` (env-overridable). Consulted by
`scripts/dev-lead-fix-reviews.sh`, `scripts/dev-lead-fix-ci.sh`, and
`scripts/review-one-pr.sh` before any automated write. On exhaustion it **stops
all automated writes**, adds `needs-human-review`, **disables auto-merge**, and
posts **one** deduped escalation (marker `<!-- pr-automation-budget exhausted -->`).
Re-engagement is human-gated. `FORCE_REVIEW` (mention-triggered) does **not**
bypass this budget.

### `petry-projects/.github` (org repo) — P0, separate PR

- **Kill the self-trigger** in `pr-review-mention-reusable.yml`: ignore mention
events authored by the bot itself and any comment carrying a
`<!-- pr-review-agent … -->` marker; the ack comment must not literally
`@`-mention `donpetry-bot` (plain text fires no mention webhook). The
`.github-private` thin-caller stub (`pr-review-mention.yml`) cannot carry this
logic (its triggers/permissions are fixed per AGENTS.md), so this half lands in
the org reusable. Land org-first so the stub never assumes undeployed behavior.

### Follow-ups (P1/P2)

- P1: `dev-lead-retry.yml` / `pr-review-sweep.yml` skip PRs carrying the
exhausted / `needs-human-review` marker before re-dispatching; post at most one
exhaustion notice per advisory engine per run.
- P2: runaway-PR detection in `daily-pr-review-health` / `scripts/fleet_monitor.sh`
(flag open PRs over soft thresholds: >50 commits, >200 comments, >N agent
cycles, or open >48h with active agent churn); prefer rebase/squash over
repeated `merge main` for agent branches.

---

## References

- Incident PR: #860
- Related: #855 (comment storm), #916 (trigger storm), #192 (retry losses),
#467 (cycle cap), #899 (same-SHA idempotency), #655 (stale-base guard)
- Implementation tracker: #926
10 changes: 10 additions & 0 deletions scripts/dev-lead-fix-ci.sh
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ source "$(dirname "$0")/engine.sh"
source "$(dirname "$0")/lib/git-identity.sh"
source "$(dirname "$0")/lib/pr-worktree.sh"
source "$(dirname "$0")/lib/auto-merge.sh"
source "$(dirname "$0")/lib/pr-automation-budget.sh"

PR_NUMBER="${PR_NUMBER:-}"
HEAD_SHA="${HEAD_SHA:-}"
Expand Down Expand Up @@ -210,6 +211,15 @@ main() {
exit 1
fi

# Per-PR automation budget (#926): if this PR has exhausted its lifetime
# automation budget since the last human interaction, stop before any writes.
# Checked before holding auto-merge so the escalation's auto-merge disable is
# not undone by the restore_auto_merge EXIT trap.
if [ "${DEV_LEAD_DRY_RUN:-false}" != "true" ] && enforce_pr_budget "$PR_NUMBER" "$REPO"; then
echo "::warning::PR #${PR_NUMBER} automation budget exhausted — skipping fix-ci"
exit 0
fi

# Hold auto-merge OFF while we work so a review approval landing mid-run can't
# merge (and delete) the branch out from under us. Install the trap and hold
# before check_idempotency so an approval landing during the API/JQ scan can't
Expand Down
11 changes: 11 additions & 0 deletions scripts/dev-lead-fix-reviews.sh
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ source "$(dirname "$0")/engine.sh"
source "$(dirname "$0")/lib/git-identity.sh"
source "$(dirname "$0")/lib/pr-worktree.sh"
source "$(dirname "$0")/lib/auto-merge.sh"
source "$(dirname "$0")/lib/pr-automation-budget.sh"

INTENT_TYPE="${INTENT_TYPE:-fix-reviews}"
PR_NUMBER="${PR_NUMBER:-}"
Expand All @@ -27,6 +28,16 @@ if [ -z "$PR_NUMBER" ] && [ "$INTENT_TYPE" != "rebase" ]; then
exit 1
fi

# Per-PR automation budget (#926): if this PR has exhausted its lifetime
# automation budget since the last human interaction, stop before any writes.
# Checked before holding auto-merge so the escalation's auto-merge disable is not
# undone by the restore_auto_merge EXIT trap. Only a human interaction resets it.
if [ -n "${PR_NUMBER:-}" ] && [ "${DEV_LEAD_DRY_RUN:-false}" != "true" ] \
&& enforce_pr_budget "$PR_NUMBER" "$REPO"; then
echo "::warning::PR #${PR_NUMBER} automation budget exhausted — skipping ${INTENT_TYPE}"
exit 0
fi

# Checkout the PR branch for modification (Requirement 1).
# Use an isolated worktree so switching to the PR branch never overwrites the
# agent's own prompts/scripts in the working tree (issue #448).
Expand Down
143 changes: 143 additions & 0 deletions scripts/lib/pr-automation-budget.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
#!/usr/bin/env bash
# pr-automation-budget.sh — per-PR automated-churn circuit breaker (issue #926).
#
# PR #860 ran away to 378 commits / 1,582 comments produced entirely by agents
# looping, with no human in the thread. Every pre-existing guard was per-worker
# or per-run; nothing bounded TOTAL automated activity on one PR over its
# lifetime, and no guard's reset required a human.
#
# This library bounds the work item (the PR), not each worker script:
# compute_pr_automation_cycles <events_json>
# Counts agent-authored activity (commits + comments + reviews/acks) that
# happened AFTER the most recent HUMAN interaction. A human action resets
# the count to zero; a machine action (bot comment, bot commit, machine
# approval) never does — so the runaway can't re-arm its own budget.
# pr_budget_exhausted <events_json>
# Exit 0 when the count has reached MAX_PR_AUTOMATION_CYCLES (default 10).
# gather_pr_automation_events <pr> <repo> -> normalized {when, login}[] JSON
# enforce_pr_budget <pr> <repo>
# The orchestration entrypoint scripts call before doing automated writes:
# on exhaustion it escalates ONCE (deduped marker + needs-human-review label
# + auto-merge disabled) and returns 0 so the caller stops. Returns 1 when
# there is still budget to proceed.
#
# Events are {when, login} objects. `when` is an ISO-8601 timestamp (strings
# compare correctly); items with null/empty `when` are ignored. A login is a
# bot when it ends in `[bot]` or is listed in AUTOMATION_BOT_LOGINS; an empty /
# unattributed login is treated as a machine (conservative — it must not reset).

# Space-separated bot logins beyond any `*[bot]` GitHub App identity. Includes
# the agent identity (donpetry-bot) and repair-pr-approvals, whose machine
# approvals must not reset the budget.
: "${AUTOMATION_BOT_LOGINS:=donpetry-bot github-actions[bot] repair-pr-approvals}"
: "${MAX_PR_AUTOMATION_CYCLES:=10}"

PR_AUTOMATION_EXHAUSTION_MARKER="<!-- pr-automation-budget exhausted -->"

_PR_BUDGET_JQ_DEFS='
def is_bot($bots):
. as $login
| ($login == null) or ($login == "")
or ($login | endswith("[bot]"))
or (($bots | index($login)) != null);
'

# compute_pr_automation_cycles <events_json>
# Prints the number of bot-authored events newer than the latest human event.
compute_pr_automation_cycles() {
local events_json="${1:-[]}"
local bots_json count
bots_json=$(printf '%s' "${AUTOMATION_BOT_LOGINS}" \
| jq -R 'split(" ") | map(select(. != ""))' 2>/dev/null) || bots_json='[]'
count=$(jq -r --argjson bots "$bots_json" "$_PR_BUDGET_JQ_DEFS"'
map(select(.when != null and .when != ""))
| ([.[] | select((.login | is_bot($bots)) | not) | .when] | max // "") as $last_human
| [.[] | select((.login | is_bot($bots)) and (.when > $last_human))]
| length
' <<<"$events_json" 2>/dev/null) || count=0
# Defensive: malformed input degrades to 0 so the integer comparison at the
# budget gate never breaks ("integer expression expected").
case "$count" in
''|*[!0-9]*) echo 0 ;;
*) echo "$count" ;;
esac
}

# pr_budget_exhausted <events_json>
# Exit 0 when the per-PR automation budget is spent.
pr_budget_exhausted() {
local events_json="${1:-[]}"
local n
n=$(compute_pr_automation_cycles "$events_json")
[ "${n:-0}" -ge "${MAX_PR_AUTOMATION_CYCLES:-10}" ]
}

# gather_pr_automation_events <pr> <repo>
# Assemble {when, login}[] from the PR's commits, issue comments, and reviews.
gather_pr_automation_events() {
local pr="$1" repo="$2"
local comments commits reviews
comments=$(gh api --paginate "repos/${repo}/issues/${pr}/comments?per_page=100" 2>/dev/null \
| jq -s '[.[][] | {when: .created_at, login: (.user?.login // "")}]' 2>/dev/null) || comments='[]'
commits=$(gh api --paginate "repos/${repo}/pulls/${pr}/commits?per_page=100" 2>/dev/null \
| jq -s '[.[][] | {when: (.commit?.author?.date // .commit?.committer?.date), login: (.author?.login // .commit?.author?.name // "")}]' 2>/dev/null) || commits='[]'
reviews=$(gh api --paginate "repos/${repo}/pulls/${pr}/reviews?per_page=100" 2>/dev/null \
| jq -s '[.[][] | {when: .submitted_at, login: (.user?.login // "")}]' 2>/dev/null) || reviews='[]'
jq -n --argjson a "${comments:-[]}" --argjson b "${commits:-[]}" --argjson c "${reviews:-[]}" \
'$a + $b + $c' 2>/dev/null || echo '[]'
}

# pr_automation_already_escalated <pr> <repo>
# Exit 0 if the exhaustion escalation comment is already present (dedupe).
pr_automation_already_escalated() {
local pr="$1" repo="$2"
gh api --paginate "repos/${repo}/issues/${pr}/comments?per_page=100" 2>/dev/null \
| jq -r '.[].body // ""' 2>/dev/null \
| grep -qF "$PR_AUTOMATION_EXHAUSTION_MARKER"
}

# pr_automation_escalate <pr> <repo>
# Post one deduped escalation, add needs-human-review, disable auto-merge.
# Re-engagement is human-gated: only a human removing the label / acting on
# the PR resets the budget (see compute_pr_automation_cycles).
pr_automation_escalate() {
local pr="$1" repo="$2"
if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ] || [ "${DRY_RUN:-false}" = "true" ]; then
echo "[dry-run] would escalate PR #${pr}: post budget-exhaustion comment, add needs-human-review, disable auto-merge"
return 0
fi
if pr_automation_already_escalated "$pr" "$repo"; then
echo "::notice::PR #${pr} already has a pr-automation-budget escalation — not re-posting"
return 0
fi
local body
body="${PR_AUTOMATION_EXHAUSTION_MARKER}
## Automated activity budget exhausted — human attention needed

This PR has reached **${MAX_PR_AUTOMATION_CYCLES}** automated actions (agent commits + review cycles + acks) since the last human interaction, without converging. To prevent a runaway loop (see #926 / the #860 post-mortem), all automated commits, reviews, and acknowledgements on this PR are now **paused**, auto-merge is disabled, and \`needs-human-review\` is applied.

**Re-engaging is human-gated.** A human reviewing, commenting, or pushing to this PR resets the budget; a machine action will not. Removing \`needs-human-review\` after a human has looked is the clean way to resume."
gh pr comment "$pr" --repo "$repo" --body "$body" \
|| echo "::warning::could not post budget-exhaustion comment on PR #${pr}"
gh pr edit "$pr" --repo "$repo" --add-label needs-human-review 2>/dev/null \
|| echo "::warning::could not add needs-human-review on PR #${pr}"
gh pr merge "$pr" --repo "$repo" --disable-auto 2>/dev/null \
|| echo "::notice::auto-merge was not enabled on PR #${pr} (nothing to disable)"
return 0
}

# enforce_pr_budget <pr> <repo>
# Returns 0 (exhausted — caller must STOP all automated writes) after
# escalating once, or 1 (budget remains — proceed).
enforce_pr_budget() {
local pr="$1" repo="$2"
[ -n "$pr" ] && [ -n "$repo" ] || return 1
local events
events=$(gather_pr_automation_events "$pr" "$repo")
if pr_budget_exhausted "$events"; then
echo "::warning::PR #${pr} reached MAX_PR_AUTOMATION_CYCLES=${MAX_PR_AUTOMATION_CYCLES} automated actions since the last human interaction — halting automation"
pr_automation_escalate "$pr" "$repo"
return 0
fi
return 1
}
30 changes: 24 additions & 6 deletions scripts/lib/review-cycle.sh
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
# All functions take a JSON array of `{when, body}` items — every review and
# comment on the PR, tagged with its timestamp (reviews' submittedAt,
# comments' createdAt). ISO-8601 timestamps compare correctly as strings.
# Items MAY also carry `author` (login) and `state` (review state) so the cap
# reset can be human-gated (#926): only a human approval resets the counter.
#
# Marker formats found in the wild (see prompts/single-review.md,
# prompts/synthesize.md, scripts/post-pr-review.sh, scripts/backfill-review-markers.sh):
Expand All @@ -21,27 +23,43 @@
# original markers inside a <details> block, so markers and timestamps remain
# meaningful for counting.

# Bot logins whose approvals are MACHINE approvals (#926): they must never reset
# the cap. Any `*[bot]` App identity is also treated as a machine.
: "${AUTOMATION_BOT_LOGINS:=donpetry-bot github-actions[bot] repair-pr-approvals}"

# Shared jq predicate definitions, prepended to every program below.
_REVIEW_CYCLE_JQ_DEFS='
def has_marker: (.body // "") | test("<!-- pr-review-agent v1 sha=[a-f0-9]+");
def is_approval: (.body // "") | test("<!-- pr-review-agent v1 sha=[a-f0-9]+\\s+decision=approved");
def is_escalation: (.body // "") | test("<!-- pr-review-agent escalation -->");
def is_bot_author($bots):
(.author // "") as $login
| ($login == "") or ($login | endswith("[bot]")) or (($bots | index($login)) != null);
# A HUMAN approval is a review with state=APPROVED authored by a non-bot. This
# is the ONLY approval that resets the cap — the cascade'\''s own approval marker
# and machine approvals (repair-pr-approvals, bot approvals) do not.
def is_human_approval($bots):
((.state // "") == "APPROVED") and ((is_bot_author($bots)) | not);
'

# compute_review_cycle <items_json>
# Prints the number of NON-CONVERGING review cycles: v1 markers that are not
# approvals, posted after the most recent reset event. Reset events:
# - the latest approval marker (the cascade converged; later cycles are a
# fresh attempt on an evolved PR, not a continuation of an old loop)
# approvals, posted after the most recent reset event. Reset events (#926 —
# the reset must be unreachable by the runaway agent, so it is human-gated):
# - the latest HUMAN approval (a review with state=APPROVED by a non-bot;
# NOT the cascade's own approval marker, NOT repair-pr-approvals/bot
# approvals — those let the loop re-arm its own cap, the #860 failure)
# - the latest `<!-- pr-review-agent escalation -->` comment (a human was
# brought in; re-engagement starts with a fresh budget)
# With no reset event present, all non-approval markers count.
compute_review_cycle() {
local items_json="${1:-[]}"
local count
count=$(jq -r "$_REVIEW_CYCLE_JQ_DEFS"'
local bots_json count
bots_json=$(printf '%s' "${AUTOMATION_BOT_LOGINS}" \
| jq -R 'split(" ") | map(select(. != ""))' 2>/dev/null) || bots_json='[]'
count=$(jq -r --argjson bots "$bots_json" "$_REVIEW_CYCLE_JQ_DEFS"'
map(select(.when != null and .when != ""))
| ([.[] | select(is_approval) | .when] | max // "") as $last_approval
| ([.[] | select(is_human_approval($bots)) | .when] | max // "") as $last_approval
| ([.[] | select(is_escalation) | .when] | max // "") as $last_escalation
| (if $last_approval > $last_escalation then $last_approval else $last_escalation end) as $reset
| [.[] | select(has_marker and (is_approval | not) and (.when > $reset))]
Expand Down
Loading
Loading