Skip to content

[Phase 4] pr-review defers indefinitely on another agent's own check run (dev-lead/dispatch) and on zombie checks — ci-pending has no timeout #1427

Closed

Description

@don-petry

Story

As a maintainer waiting for pr-review to approve a PR whose tests are green,
I want the "CI still pending" deferral to ignore other agents' own check runs and to time out on a check that never completes,
so that ordinary dev-lead activity — or a single stuck check — cannot defer the review indefinitely.

Problem — two ways a PR gets stuck at ci-pending forever

scripts/review-one-pr.sh skips with reason:"ci-pending" whenever compute_ci_status sees a non-completed check, posting a <!-- pr-review-agent ci-pending-ack --> comment that says "re-mention @donpetry-bot once they complete." scripts/lib/ci-status.sh deliberately filters the cascade's own check runs (review, review / review, PR Review*) so pr-review never blocks on itself (#469). It does not filter any other agent's checks, and the pending state has no upper bound — a FORCE_REVIEW mention polls only 10×30s before giving up.

Case A — dev-lead's own check defers pr-review (agent-vs-agent interference)

Observed on PR #1420 at 07:48Z: 57 checks COMPLETED, all tests green (bats, unit, unit-tests all SUCCESS), zero unresolved threads, zero genuine failures — and one check IN_PROGRESS: dev-lead / dispatch, started 07:39:01Z. pr-review posted ci-pending-ack at 07:45:08Z and declined to review.

The loop is self-sustaining, and it is easy to enter by trying to help:

  1. Someone comments on the PR (including "@donpetry-bot review", the remedy the ack itself recommends)
  2. The comment is an issue_comment event → dev-lead fires → dev-lead / dispatch check goes IN_PROGRESS
  3. pr-review sees a pending check → defers → posts another ci-pending-ack
  4. Because dev-lead's concurrency is cancel-in-progress: false, queued runs keep at least one dispatch check pending across the window, so step 3 repeats

Every attempt to re-engage the reviewer re-arms the condition that makes it defer. During the observed window, two mention-driven attempts to obtain an approval both ended in ci-pending-ack rather than a review.

Case B — a zombie check never completes

Observed on PR #1426 at 07:48Z: Validate AW specs, docs, and workflows reports status: IN_PROGRESS with conclusion: SUCCESS, started 06:57:49Z — inconsistent, and ~50 minutes stale. A check in this state will not transition, so the ci-pending skip is permanent: pr-review will defer on every future trigger, forever, with no timeout to rescue it.

Both PRs were otherwise fully merge-ready. Nothing has merged since 05:49Z.

Acceptance Criteria

  1. compute_ci_status excludes other first-party agent check runs from the pending calculation — at minimum dev-lead / *, generalised from the existing self-filter rather than hard-coded per agent. A writing agent's own job must not gate the reviewing agent.
  2. The exclusion is derived from a declared source (the interaction contracts from [Phase 2] Author a machine-readable interaction contract for every agentic role, per the standard #1404 name each role's workflows), not a second hand-maintained name list that can drift from the first.
  3. The ci-pending deferral is bounded: after a configurable age, pr-review either proceeds on the completed checks or escalates once, rather than deferring indefinitely. A check that is IN_PROGRESS while carrying a terminal conclusion is treated as complete.
  4. The bound is fail-safe, not fail-open: proceeding past a genuinely pending required check must remain impossible — the timeout applies to the review decision, not to the merge gate, which the rulesets still enforce independently.
  5. The ci-pending-ack comment stops recommending a remedy that re-triggers the condition. Either the ack is dropped for the self-inflicted case, or its text stops advising a re-mention when the only pending check belongs to another agent.
  6. Tests cover: pending dev-lead check + otherwise-green PR → review proceeds; zombie check (IN_PROGRESS + conclusion) → treated as complete; a genuinely pending required check → still defers; the deferral timeout path.

Tasks / Subtasks

Dev Notes

Project Structure Notes

scripts/lib/ci-status.sh (the filter and the pending calculation), scripts/review-one-pr.sh (the skip path and the ack copy); tests under tests/, registered in lint.yml's bats list.

References

Likely target surface

  • scripts/lib/ci-status.sh, scripts/review-one-pr.sh

Filed from overnight monitoring of epic #1402. Found while diagnosing why two fully-green PRs would not attract a code-owner approval: the reviewer was deferring on the writing agent's own check run, and each attempt to re-engage it re-armed that deferral.

Activity

  1. github-actions commented on Aug 2, 2026

    @github-actions
    Contributor

    Thanks for the exceptionally thorough write-up — the two observed fixture PRs, the self-reinforcing loop diagram, and the zombie-check edge case make the failure modes very clear. Before we move to implementation, one question on AC #2: the interaction contracts from #1404 are listed as the intended source for the agent-check filter — are those contracts currently stored in a machine-readable format (e.g., structured YAML/JSON) that ci-status.sh can parse directly, or would sourcing from them require reading prose documentation first? That'll determine whether the generalised filter can be data-driven from day one or needs an intermediate step.

  2. don-petry commented on Aug 2, 2026

    @don-petry
    CollaboratorAuthor

    Trigger-source data, and a hypothesis I checked and discarded

    Discarded: a check_run self-trigger loop. I suspected dev-lead was re-triggering itself via its own dev-lead / * check runs (it creates four on a PR and subscribes to check_run). It is not that. The ci-relay job's guard requires conclusion == 'failure' and !startsWith(name, 'dev-lead / ') (.github/workflows/dev-lead-reusable.yml:424-428), and dev-lead's own checks complete skipped/success — so they cannot relay. 8 check_run-triggered runs in an hour on a PR carrying 57+ checks is ordinary. Recording the negative result so nobody re-derives it.

    What the data does show. dev-lead runs on PR #1420 in the 07:20–08:17 window, by trigger:

    event runs
    issue_comment 10
    check_run 8
    pull_request_review 2
    pull_request 2
    issues 1
    total 23

    There were 6 comments on the PR in that window. So comments are the single largest trigger source, and the reviewer's own activity is the dominant contributor — my two @donpetry-bot review mentions plus dev-lead's own marker comments each start a dev-lead run. Note a run starts before the skip decision is made: dev-lead-intent.sh skips comments authored by BOT_USER (:376-377), but only after the workflow has already begun and created its dev-lead / dispatch check.

    Why that matters for this issue. Each of those runs holds a dev-lead / dispatch check IN_PROGRESS, and per AC #1 that is what defers pr-review. So the failure mode is self-reinforcing through ordinary review traffic rather than through any loop: commenting on a PR to ask for a review is itself the thing that defers the review. On #1420 the two mention attempts both ended in ci-pending-ack instead of a review, and donpetry-bot has still never reviewed that PR.

    Immediate mitigation, no code required: stop commenting and let the dev-lead queue drain, at which point no dev-lead check is pending and pr-review can proceed. I am doing that now for #1420 and #1426. That is a workaround, not a fix — AC #1 remains the fix, because "reviewers must stay silent for the reviewer to work" is not a usable contract.


    Generated with Claude Code

  3. don-petry commented on Aug 2, 2026

    @don-petry
    CollaboratorAuthor

    Mitigation confirmed empirically — going quiet unblocked both PRs

    Recording the result, because it is the strongest evidence yet for AC #1.

    At 08:17Z both #1420 and #1426 had been stuck for hours: fully merge-ready (green tests, zero unresolved threads, zero genuine failures) but each holding one non-completed check — dev-lead / dispatch on #1420, the zombie Validate AW specs on #1426 — so pr-review kept answering with ci-pending-ack instead of reviewing. donpetry-bot had never reviewed #1420 across roughly three hours and two mention attempts.

    I stopped commenting on both. Result:

    PR outcome
    #1420 merged 08:32:02Z (~15 min after the last comment)
    #1426 approved 08:39:57Z, merged 08:39:59Z (~22 min)

    Nothing else changed — no code push, no configuration change, no human intervention. The only variable was the absence of new comments, which let the dev-lead queue drain so that no dev-lead check was pending, which let pr-review proceed.

    This confirms the diagnosis and inverts the ergonomics. The remedy the ci-pending-ack itself recommends — "re-mention @donpetry-bot" — is counter-productive whenever the pending check belongs to dev-lead: the mention starts a dev-lead run whose dispatch check is pending, which is precisely what defers the review. Silence is the effective remedy and the ack advises the opposite. That is AC #5's concern, now demonstrated rather than argued.

    It also means the failure is self-limiting on a quiet PR and self-sustaining on an active one — it bites hardest exactly when a PR is under active review, which is when it matters most.

  4. don-petry commented on Aug 2, 2026

    @don-petry
    CollaboratorAuthor

    Maintainer decision — full fix: exclude agent checks + bounded deferral

    Scope confirmed by the maintainer. Implement the complete AC set:

    The empirical confirmation is already on this issue: going silent merged #1420 in ~15 min and #1426 in ~22 min with no other change. That is the behaviour the fix must make unnecessary — "the reviewer must stay silent for the reviewer to work" is not a usable contract.

    Unblocking for implementation.

  5. don-petry commented on Aug 2, 2026

    @don-petry
    CollaboratorAuthor

    Additional AC — a non-required check failure silently halts all review

    Same file, same class of defect as this issue's pending-deferral, so folding it in rather than filing separately.

    Observed now on PR #1434. It has no pending checks and zero unresolved threads, yet donpetry-bot has never reviewed it. The cause is one failing check — template-drift — which is:

    compute_ci_status (scripts/lib/ci-status.sh:20,85) returns failing for anything that is not SUCCESS / SKIPPED / CANCELLED, with no notion of required. review-one-pr.sh:141 then skips — and that skip is silent: it writes only to the run log, posting nothing on the PR. So an unrelated external drift stops every review in the repo with no signal anywhere the reader will look.

    This is the same failure shape as the pending-check deferral: pr-review declines to act on a signal that should not gate it, and says nothing.

    Proposed AC #7

    1. A non-required failing check does not by itself prevent a review. compute_ci_status distinguishes required from non-required checks (the ruleset's required set is queryable), and pr-review gates only on the required ones. Where a non-required failure does cause a skip, the skip is visible — one deduped marker naming the check — rather than silent, so a reader can tell why nothing is happening.

    Note the existing break-glass (FORCE_REVIEW bypasses ci-failing, #619) is documented for exactly this case — "a fix to the CI gate itself can be reviewed" — but it requires a human mention, and per this issue's own findings a mention re-triggers dev-lead and can re-arm the pending deferral. The break-glass should be the exception, not the routine remedy for an unrelated red check.

    Separate follow-up, outside this epic: the repo-template drift itself needs a re-seed via scripts/seed-repo-template.sh. That is maintenance in another repo, not an interaction-model defect.

  6. don-petry commented on Aug 2, 2026

    @don-petry
    CollaboratorAuthor

    The cause of the template-drift failure cited in AC #7 above is now filed separately as #1435: Dependabot is bumping action pins inside repo-template, a byte-identity distribution artifact, so the shipped files drift from the standards-derived baseline by construction (the 09:49Z actions/checkout 6.0.2→7.0.1 bump landed five minutes before drift began failing; standards pins v7.0.0).

    The two issues are complementary and both wanted: #1435 stops the drift being generated; AC #7 here stops a non-required red check from silently halting every review. Either alone leaves half the failure — a different non-required check will eventually go red for an unrelated reason, and without AC #7 the same silent halt recurs.

  7. don-petry commented on Aug 2, 2026

    @don-petry
    CollaboratorAuthor

    The #619 break-glass did not bypass ci-failing when dispatched — observed, mechanism unconfirmed

    Relevant to AC #7. I tried to review PR #1434 through the documented break-glass and it did not work.

    What I ran:

    gh workflow run pr-review-trigger.yml \
      -f pr_url=https://github.com/petry-projects/.github-private/pull/1434 \
      -f dry_run=false -f force_review=true
    

    What the run decided (run 30749802155, workflow_dispatch, conclusion success):

    review decision: REVIEW_REQUIRED
    skip: CI checks are failing — will re-evaluate after fixes are pushed
    {"pr":".../pull/1434","sha":"c2c43a2b…","decision":"skip","reason":"ci-failing"}
    

    That is the else branch at scripts/review-one-pr.sh:156 — the path taken when FORCE_REVIEW is not true. The force-review branch (:142-154) emits a distinctive ::warning::force-review: CI is failing but FORCE_REVIEW=true …, which does not appear anywhere in the run log.

    What I checked and could not fault:

    • pr-review-trigger.yml declares the force_review dispatch input and forwards it: force_review: ${{ inputs.force_review || '' }}.
    • The reusable declares force_review as a workflow_call input.
    • The env wiring is identical at main and at the pinned channel @pr-review/next (ded84ce4820d): FORCE_REVIEW: ${{ github.event_name == 'repository_dispatch' && 'true' || inputs.force_review || 'false' }}.

    So the plumbing reads correctly end-to-end, yet the script behaved as though FORCE_REVIEW=false. I am not asserting a mechanism — I could not confirm the value inside the run before hitting an API error, and I have been wrong twice tonight when I inferred a mechanism without proof. Recording it as a reproducible observation for whoever picks up this issue.

    Why it matters here. AC #7 argues that a non-required red check should not silently halt review, and notes the break-glass exists for exactly this case. If the break-glass does not work from workflow_dispatch, then there is currently no working escape from a ci-failing skip other than making the unrelated check green — which is precisely the situation that has held this repo at zero merges since 09:55Z. That strengthens AC #7 from "nice to have" to "the only remaining lever".

    Worth a targeted reproduction: dispatch with force_review=true against any PR with a red non-required check and confirm whether the force-review warning appears.

  8. don-petry commented on Aug 12, 2026

    @don-petry
    CollaboratorAuthor

    Dev-Lead Implementation Plan

    Issue: #1427 — [Phase 4] pr-review defers indefinitely on another agent's own check run (dev-lead/dispatch) and on zombie checks — ci-pending has no timeout

    Scope

    Stop compute_ci_status from gating pr-review on another first-party agent's own orchestration check runs (sourced from the interaction-contracts/), treat a check carrying a terminal conclusion as complete, and bound the ci-pending deferral so a genuinely stuck check can no longer defer the review forever.

    Implementation Checklist

    Tests to Write

    • tests/test_ci_status.bats: dev-lead / dispatch (IN_PROGRESS) + otherwise-green → passing (filtered); zombie (IN_PROGRESS + conclusion: SUCCESS) → treated complete; genuine external pending required check → still pending; ci_pending_age_exceeded true/false paths (update the now-outdated "dev-lead NOT filtered" cases)
    • tests/test_review_one_pr_ci_pending.bats (or a new bats): stuck pending → review proceeds (no ci-pending skip); ack copy contains no re-mention advice

    Test Command

    bats tests/test_ci_status.bats tests/test_review_one_pr_ci_pending.bats and bash .dev-lead/scripts/dev-lead-lint.sh

  9. don-petry commented on Aug 12, 2026

    @don-petry
    CollaboratorAuthor

    ✅ Issue #1427 — pr-review ci-pending deferral bounded + first-party agent checks de-gated

    Plan Execution

    Test Results

    bats tests/*.bats  → 1661 tests, 0 failures (RC=0)
    scripts/dev-lead-lint.sh → passed
    shellcheck → clean (info-level SC2016/SC1091 only, matching existing sibling patterns)
    

    New/updated coverage (AC #6):

    • pending dev-lead / * check + otherwise-green PR → proceeds (filtered).
    • zombie IN_PROGRESS+SUCCESS → passing; IN_PROGRESS+FAILURE → failing; zombie + a genuine pending → pending.
    • genuinely-pending required check → still defers.
    • stuck pending (started ~2h ago) → proceeds past the gate + posts ci-pending-timeout; escalation copy contains no re-mention.
    • ci_pending_age_exceeded unit tests (recent/old/unknown-timestamp/multi-check).

    Files Changed

    File Change
    scripts/lib/ci-status.sh agent-check filter derived from contracts; terminal-conclusion-wins; new ci_pending_age_exceeded; classify_rollup_eventability filter parity
    scripts/review-one-pr.sh bounded ci-pending deferral + one-shot ci-pending-timeout escalation; revised ci-pending-ack copy
    tests/test_ci_status.bats agent-filter, zombie, required-pending, and age-check tests
    tests/test_review_one_pr_ci_pending.bats stuck-pending proceed/escalate + ack copy guards
    tests/test_review_one_pr_force_ci_failing.bats fixture updated from now-filtered dev-lead / dispatch to genuine external CI / build FAILURE

    Notes

    • Design choice (AC Go-live improvements for PR review agent #2): agent roles are read from interaction-contracts/*.yml role: fields and matched against the nested reusable-workflow check shape <role> / <job> (e.g. dev-lead / dispatch). This is contract-sourced, so onboarding a new agent needs no change here. Bare context names like dispatch/ci-relay are intentionally left unfiltered to avoid swallowing a genuine external check that happens to share a job name.
    • The test_review_one_pr_force_ci_failing.bats fixture was updated because its old dev-lead / dispatch FAILURE check is now (correctly) filtered out of the CI gate — the break-glass path under test only triggers on real failing CI, so it needed a genuine external failing check.
    • Per the issue Dev Notes, Case A was not addressed by giving dev-lead cancel-in-progress: true; the fix lives entirely in what pr-review counts.

    🤖 dev-lead

  10. added a commit that references this issue on Aug 12, 2026
  11. added a commit that references this issue on Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugBug reportsdev-leadFor dev-lead agent pickupinitiativeEpic / initiative tracking issue

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions