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

[don-petry]

Problem

dev-lead's fix-review only acts on review threads/findings (advisory-bot comments). When dev-lead's own change breaks a CI check (e.g. a red bats assertion or shellcheck warning) that has no associated review finding, the fix-review returns status=no-changes and the PR sits BLOCKED indefinitely — dev-lead won't fix a regression it introduced because the failing check isn't a 'finding' it recognizes.

Concrete case (2026-07-23)

PR #887 (implementing #886) regressed Lint and bats:

• not ok 26 opens a PR to INSERT the baseline when the repo has no .gitignore — was green on main (last run 2026-07-20, sha 49ecdfb: ok 26), red on feat: implement issue #886 — [#850] repo-template baseline re-seed pins BARE @dev-lead/stable — reverts the v<M>-stable convergence (flapping) #887.
• dev-lead ran two fix-review cycles on feat: implement issue #886 — [#850] repo-template baseline re-seed pins BARE @dev-lead/stable — reverts the v<M>-stable convergence (flapping) #887, both status=no-changes (intent=review-changes, intent=fix-bot-comment). It never addressed the failing test.
• Result: PR stuck (BLOCKED, red check, no new commit, not even needs-human-labeled). A human had to intervene.

Expected

dev-lead's fix loop should treat a required CI check that its own commits turned from green→red as a first-class thing to fix — diff the failing check against the PR base, reproduce the failure, and fix it (or escalate to needs-human with the specific failing test named). Silent no-changes on a self-introduced red check is the worst outcome (looks 'done', actually stuck).

Scope

• In the fix-review loop (dev-lead-fix-reviews / review-cycle), detect failing required checks (not just review threads); if a check regressed vs base and there are no findings, attempt a fix or set needs-human with the failing-check detail — never exit no-changes while a required check is red.
• Add a guard/test for: 'required check red + no review findings' → must NOT resolve as no-changes.

Refs: #886/#887 (the incident), Epic #850.

[don-petry]

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.)

[don-petry]

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

[don-petry]

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.