[dev-lead timeout follow-up 2/3] Dedup handle_engine_failure escalation branches into a shared helper #1029

Activity

1. don-petry

   added

   enhancementFeature requestsFeature requests

   on Jul 3, 2026
2. github-actions commented on Jul 3, 2026

   

   github-actionsBot

   on Jul 3, 2026 – with GitHub Actions

   Contributor

   More actions

   Thanks for the detailed write-up and for linking the upstream context — this is a well-scoped refactor with clear acceptance criteria. Before diving in: should escalate_needs_human be defined as a top-level function in dev-lead-fix-issue.sh, or is there a preferred shared-library pattern (e.g., a sourced scripts/lib/ file) that other scripts in this repo already use for helpers?
3. don-petry

   added

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

   on Jul 3, 2026
4. don-petry commented on Jul 3, 2026

   

   don-petry

   on Jul 3, 2026

   CollaboratorAuthor

   More actions

   Dev-Lead Implementation Plan

   Issue: #1029 — [dev-lead timeout follow-up 2/3] Dedup handle_engine_failure escalation branches into a shared helper

   Scope

   Extract a single escalate_needs_human helper in scripts/dev-lead-fix-issue.sh that owns the five common escalation steps (::error::, ensure_needs_human_label, the byte-identical status=needs-human marker comment, run link + redacted snippet, then exit), and route the missing-binary / timeout / retries-exhausted branches through it — passing only their reason-specific cause text. Pure refactor, no marker/behavior change.

   Implementation Checklist

   • Add escalate_needs_human <reason> <attempt> <snippet> <error_line> <cause_markdown> [exit_code] helper (byte-identical marker + shared heading/label/snippet/exit)
   • Route the missing-binary branch through the helper (cause text only)
   • Route the timeout branch through the helper (retains tier/elapsed/budget/split-or-raise/work-note specifics)
   • Route the retries-exhausted branch through the helper (exit 2 preserved when engine_rc==2)

   Tests to Write

   • New assertion in tests/dev-lead/unit/test_fix_issue.bats: all three escalation branches emit the same needs-human marker shape (only reason= differs)
   • Confirm existing missing-binary / timeout / retries-exhausted / attempt-ceiling escalation tests still pass unchanged

   Test Command

   bats tests/dev-lead/unit/test_fix_issue.bats (full unit dir: bats tests/dev-lead/unit/); lint via bash scripts/dev-lead-lint.sh
5. don-petry commented on Jul 3, 2026

   

   don-petry

   on Jul 3, 2026

   CollaboratorAuthor

   More actions

   Dev-Lead: Implementation Complete

   Plan Execution

   • Added escalate_needs_human <reason> <attempt> <snippet> <error_line> <cause_markdown> [exit_code] helper — single source for the five common escalation steps (::error::, ensure_needs_human_label, byte-identical status=needs-human marker comment, redacted snippet, exit)
   • Routed the missing-binary branch through the helper (cause text only)
   • Routed the timeout branch through the helper — retains its tier / elapsed / budget / split-or-raise guidance / completed-but-unpushed work note
   • Routed the retries-exhausted branch through the helper, preserving the exit-2-on-rate-limit contract via the helper's exit_code arg
   • Added a bats assertion that all three branches emit the same needs-human marker shape (only reason= differs)

   No behavior change

   The emitted marker <!-- dev-lead-issue N status=needs-human attempt=K reason=X run=ID --> is byte-identical to before, so dev-lead-retry.sh (skips status=needs-human) and the #1019 fleet-monitor (summarize_dev_lead_timeouts parses reason=) are unaffected. The only cosmetic shift: the trailing human guidance in the missing-binary / retries-exhausted comments now renders above the redacted snippet instead of below it, matching the canonical cause + run link + snippet order (the timeout comment was already in that order → byte-identical).

   Test Results

   tests/dev-lead/unit/test_fix_issue.bats — 20/20 passing (was 19; +1 new marker-shape guard)
   1..20
   ok 14 fix-issue: missing-binary → escalates to needs-human, no retry marker
   ok 15 fix-issue: stage timeout (exit 124) → escalates to needs-human, reason=timeout, no retry marker
   ok 18 fix-issue: attempt ceiling (prior attempt=2) → escalates to needs-human
   ok 19 fix-issue: all three escalation branches emit the same needs-human marker shape (only reason differs)
   ok 20 fix-issue: lint fails → posts lint-failure comment on the issue
   
   bash scripts/dev-lead-lint.sh — all checks passed (shellcheck clean on 85 files + agent-profile/CODEOWNERS validation)
   

   Files Changed

   • scripts/dev-lead-fix-issue.sh: extracted escalate_needs_human helper; the three non-retryable branches now call it with only their reason-specific cause text.
   • tests/dev-lead/unit/test_fix_issue.bats: added the cross-branch marker-shape assertion (AC4).

   Notes

   • Pre-existing unrelated failure: tests/dev-lead/unit/test_engine_mcp.bats refactor: migrate to machine user PAT auth (closes #27) #28 (REVIEW_MCP_DEBUG set + healthy handshake …) fails on a clean checkout with my changes stashed — it is not caused by this refactor and is out of scope for [dev-lead timeout follow-up 2/3] Dedup handle_engine_failure escalation branches into a shared helper #1029.
   • Marker parsers (dev-lead-retry.sh, fleet_report.sh) were read to confirm the status=needs-human / reason= contract; both remain satisfied.
6. don-petry

   added 2 commits that reference this issue on Jul 3, 2026

   fix(escalation): guard escalate_needs_human arg count under set -u (#…

   ...

   8c3ac9e

   feat: implement issue #1029 — [dev-lead timeout follow-up 2/3] Dedup …

   ...

   83fc777
7. don-petry

   closed this as completedin #1035on Jul 3, 2026