dev-lead fix-review ignores CI-check regressions it introduced (red bats/lint with no review finding → status=no-changes, PR stuck) #888

Activity

1. don-petry

   added

   dev-leadFor dev-lead agent pickupFor dev-lead agent pickup

   on Jul 23, 2026
2. don-petry commented on Jul 23, 2026

   

   don-petry

   on Jul 23, 2026

   ContributorAuthor

   More actions

   Correction on the incident attribution (systemic point unchanged): on closer analysis, PR#887 did NOT introduce the red Lint and bats check — the failing test (compliance-remediate gitignore-baseline test 26) was already failing on main, a pre-existing regression from #865 (a preflight permissions.push write-probe + sha-unresolved guard added to the shared sd_deploy_files_via_pr lib whose compliance-remediate test mock was never updated). #887 merely re-runs that suite via its path trigger and surfaced it.

   The systemic dev-lead problem this issue tracks still holds, and is arguably worse: dev-lead ran two fix-review cycles on a PR with a red required check and both returned status=no-changes. Whether the red check was self-introduced OR pre-existing-and-surfaced, dev-lead should never exit no-changes while a required check is failing — it should reproduce/fix it or escalate needs-human naming the failing test, not silently look 'done'. (Fixed the mock directly on #887 to unblock; the test regression itself is worth back-fixing on main independent of #887.)
3. don-petry commented on Jul 23, 2026

   

   don-petry

   on Jul 23, 2026

   ContributorAuthor

   More actions

   Dev-Lead Implementation Plan

   Issue: #888 — dev-lead fix-review ignores CI-check regressions it introduced (red bats/lint with no review finding → status=no-changes, PR stuck)

   Scope

   In the fix-review loop, treat a red required CI check as a first-class blocker even when there is no associated review finding: the non-retryable fix-bot-comment intent must never post status=no-changes while a required check is red — it escalates to needs-human naming the failing check(s) instead.

   Root cause

   scripts/dev-lead-fix-reviews.sh already gates the retryable intents (fix-reviews, review-changes) on has_hard_blockers → they post a rate-limited/blocked retry marker, not no-changes. But the non-retryable fix-bot-comment path (lines ~1318-1333) calls post_no_changes on every branch — including when a required check is red — so the PR is left looking "done" while actually BLOCKED. There is also no path that names the failing check or applies needs-human-review.

   Implementation Checklist

   • scripts/dev-lead-fix-reviews.sh: add pure, sourceable failing_required_check_names() — prints red check names (conclusion failure/timed_out/action_required/startup_failure/stale) from CI_STATUS_JSON; excludes pending/cancelled.
   • scripts/dev-lead-fix-reviews.sh: add escalate_needs_human_red_checks(intent, names) — posts a terminal status=needs-human marker naming the failing check(s) and adds the needs-human-review label (reusing NEEDS_HUMAN_REVIEW_LABEL); dry-run echoes.
   • scripts/dev-lead-fix-reviews.sh: wire the guard into the fix-bot-comment no-changes path — red required check present → escalate needs-human instead of post_no_changes.

   Tests to Write

   • Unit: failing_required_check_names — red check → prints name; all-success → empty; cancelled/pending → excluded.
   • Guard: fix-bot-comment with a red required check and no review findings/threads → output must NOT contain status=no-changes; must contain status=needs-human and the failing check name.
   • Update existing test "fix-bot-comment with hard blockers posts terminal no-changes" to assert the new needs-human escalation (red check present) instead of silent no-changes.

   Test Command

   bats tests/dev-lead/unit/test_fix_reviews.bats (run from the .github-private checkout), plus bash scripts/dev-lead-lint.sh
4. don-petry commented on Jul 23, 2026

   

   don-petry

   on Jul 23, 2026

   ContributorAuthor

   More actions

   Dev-Lead: Implementation Complete

   Plan Execution

   • Added pure failing_required_check_names() (red check names from CI_STATUS_JSON; excludes pending/cancelled)
   • Added escalate_needs_human_red_checks() (names checks, applies needs-human-review, posts status=needs-human terminal)
   • Wired the guard into the non-retryable fix-bot-comment no-changes path — red check ⇒ escalate needs-human instead of a done-looking no-changes
   • Added guard tests + updated the prior "posts terminal no-changes" expectation to the new escalation
   • Full test_fix_reviews.bats suite + dev-lead-lint.sh (shellcheck) pass

   Root cause

   The retryable intents (fix-reviews, review-changes) already gate on has_hard_blockers → they post a rate-limited/blocked retry marker (an implicit re-attempt), never no-changes, when a required check is red. But the non-retryable fix-bot-comment path called post_no_changes on every branch — including when a required check was red — so the PR looked "done" while sitting BLOCKED, with nothing to auto-retry it. That is the exact green→red-with-no-finding stall from PR #887.

   Fix

   In the fix-bot-comment no-changes branch, a red required check now short-circuits to escalate_needs_human_red_checks, which:

   • names the failing check(s) in a visible comment,
   • applies the needs-human-review label, and
   • posts a status=needs-human terminal marker (not status=no-changes), so the stuck state is visible and a human is pulled in rather than the PR looking finished.

   "Red" = failure|timed_out|action_required|startup_failure|stale; pending (no verdict yet) and cancelled (non-blocking, #608) are deliberately excluded so a superseded/queued check never triggers a false escalation.

   Test Results

   test_fix_reviews.bats: 82 passing (incl. 3 new #888 tests below), 8 pre-existing
   failures — all in the non-dry-run "rate-limited" group that needs live gh/git auth
   (unrelated to this change; identical before the change on a clean checkout).
   
   ok 58 fix-bot-comment with a red required check escalates needs-human (not silent no-changes)
   ok 61 fix-bot-comment: red required check with no review findings escalates needs-human, never no-changes (#888)
   ok 62 fix-bot-comment: all-green checks with no findings still posts no-changes (no false needs-human)
   ok 63 fix-bot-comment: lone cancelled check is not treated as red — no needs-human escalation
   
   dev-lead-lint.sh: shellcheck on 79 files + agent/CODEOWNERS validation → all checks passed
   

   Files Changed

   • scripts/dev-lead-fix-reviews.sh: add failing_required_check_names() + escalate_needs_human_red_checks(); guard the fix-bot-comment no-changes path.
   • tests/dev-lead/unit/test_fix_reviews.bats: 3 new guard tests; updated the prior fix-bot-comment hard-blocker test to assert needs-human escalation.

   Notes

   • Scope: the deterministic guard the issue requests ("required check red + no review findings → must NOT resolve as no-changes") is implemented for the path that violated it (fix-bot-comment). The retryable intents already satisfy it via the existing has_hard_blockers retry gate.
   • Pure timing (a check that flips red after assessment) is not addressed here — that would need a re-fetch/base-diff and is out of scope for the deterministic guard.
   • The needs-human terminal is intentionally sticky (not matched by the applied|no-changes|failed stale-marker expiry, and not auto-retried); a human removing needs-human-review / pushing a fix resumes automation on the new SHA.
5. don-petry

   added a commit that references this issue on Aug 7, 2026

   docs(ci-standards): gate-label creation in the enrollment checklist (#…

   ...

   baf01e9
6. don-petry

   added a commit that references this issue on Aug 31, 2026

   docs(ci-standards): gate-label creation in the enrollment checklist (#…

   ...

   d6a2943
7. don-petry

   added a commit that references this issue on Sep 7, 2026

   docs(ci-standards): gate-label creation in the enrollment checklist (#…

   ...

   e2562b1