fix(acp): retry transient connection failures

[Premshay]

Problem / Motivation

ACP connection failures such as refused/reset/timeouts and plain service-unavailable responses are currently terminal, though they can clear without user intervention.

Why it matters

A transient backend or transport outage ends a session unnecessarily instead of using the existing bounded retry path.

What changed

Classify common connection failures and plain 5xx service-unavailable responses as retryable. Authentication, quota, and session-expiry classifications retain precedence. The surfaced fallback now tells the user that the backend could not be reached.

Tests

python -m pytest test/test_acp_error_surface.py test/test_acp_client.py -n0 -q — 651 passed.

What changed (motivation → approach → change)

N/A — covered by the existing ## What changed section.

Manual verification

N/A — focused automated coverage is sufficient.

Related Issues

N/A.

Checklist

• Existing tests pass and regression coverage is included.
• Self-review completed; code follows project style guidelines.
• Documentation updated where applicable.
• No secrets, credentials, or internal references in the diff.

Contribution License Agreement

N/A — template placeholder; no CLA wording is supplied.

Pattern harvest

Not generalizable: this is a bounded adapter-startup retry at a specific process-creation boundary.

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of a9ea9ee3aa4b91ad6cbc51ac0a996038c3e56f5f via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The design is sound: right layer (the existing raw-error classifier that both the formatter and retry verdict key off), terminal branches keep precedence, the retry budget is already bounded, and ENOTFOUND-vs-EAI_AGAIN shows deliberate transient/permanent discrimination. The only asymmetry I found: _RE_CONNECTION matches prose forms ("connection refused/error/closed/timed out") that the new _TRANSIENT_MARKERS entries don't carry, so the string-fallback path (legacy raises without the .transient flag) can still classify those as terminal — a drift the module's own comments warn against, but with a bounded, pre-PR-equivalent consequence.

Design-Verdict: PASS

A real transient-failure gap, closed at the established classifier seam with bounded retries and terminal precedence preserved — proportionate and reversible.

Suggestions

• _TRANSIENT_MARKERS gained the errno tokens but not _RE_CONNECTION's prose forms ("connection refused/error/closed/timed out"), so a restored raw error using those spellings stays terminal on the string-fallback path; add them (or derive the markers from the regex) to honor the module's "never drift again" coupling.

[DESIGN-REVIEWED] a9ea9ee

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed a9ea9ee3aa4b91ad6cbc51ac0a996038c3e56f5f via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] a9ea9ee

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of a9ea9ee3aa4b91ad6cbc51ac0a996038c3e56f5f via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence is gathered: the patch adds a connection-failure classification to an existing retry classifier; base _RE_5XX_NAMED (client.py:1815) already covers "connection reset" and "service unavailable"; _RE_5XX_NAMED has exactly 2 consumers, both of which now check the new regex first. Here is the review.

First-Principles-Verdict: CONCERNS

Node-style tokens ("socket hang up", "fetch failed", ECONNREFUSED) rest on no captured error frame — no issue, no observed string, only author-invented test inputs.

Not justified as shipped

3. rides along, undeclared — connection-reset errors were already retryable via _RE_5XX_NAMED (client.py:1818); only their message wording changes.

What this change ships

Intent: keep a session alive through a momentary backend/transport outage by letting the existing bounded retry ladder absorb connection failures — a FIX.

1. Refused/reset/timed-out connections now retry on the existing bounded ladder instead of ending the turn — justified
2. Those failures surface as "Could not reach the model backend…" instead of the raw unknown-error fallback — justified
3. Connection-reset errors swap the generic HTTP-5xx message for the new wording — rides along, undeclared
4. Restored/legacy-path error text with raw errno tokens classifies transient in the outer retry layer — justified

Watch

• The vocabulary is an unverified provider claim: nothing shows kiro-cli or any ACP backend emitting "socket hang up"/"fetch failed"/"connect ECONNREFUSED" inside a JSON-RPC error frame; the added tests fail on base but use invented strings, and "Related Issues: N/A". Wrong premise is inert (errors stay terminal), so reversible. Clears when: one real captured error frame or linked issue per vocabulary family (Node errno, prose "connection …") is named.
• Description overclaims: "Classify … plain 5xx service-unavailable responses as retryable" — base _RE_5XX_NAMED already does (client.py:1817); and the "Pattern harvest" line describes an "adapter-startup retry at a process-creation boundary" this diff does not contain. Clears when: the description drops both.

Subtractions

• Drop the now-shadowed connection{_5XX_SEP}reset(?:{_5XX_SEP}error)? alternation from _RE_5XX_NAMED (client.py:1818) — its only 2 consumers (client.py:2069, 2350) now match _RE_CONNECTION's connection reset first, leaving two spellings to diverge.

[FIRST-PRINCIPLES-REVIEWED] a9ea9ee

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed a9ea9ee3aa4b91ad6cbc51ac0a996038c3e56f5f via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] a9ea9ee

[bolichen97]

@Premshay Thanks for this. A read-only audit of all open PRs found two other open PRs working in the same code, so a short reconciliation now would save a painful rebase later.

#9330 ("classify prose-spelled dispatch failure as transient", by @ShortEmperor) widens the same classifier, _is_transient_raw_error in src/kiro_crew/acp/client.py, and edits the same _RE_5XX_NAMED block your new regexes sit next to. It rewrites _RE_5XX_NAMED into a case-insensitive, separator-tolerant pattern covering internal server error, service unavailable, dispatch failure and connection reset. Once that lands, your _RE_5XX_PHRASE is a strict subset of it, and the "connection reset" wording it adds is already matched by your _RE_CONNECTION. Everything else in this PR is unique and worth keeping: the errno vocabulary (ECONNREFUSED, ECONNRESET, ECONNABORTED, ETIMEDOUT, EPIPE, EHOSTUNREACH, EAI_AGAIN, "socket hang up", "fetch failed"), the ENOTFOUND-stays-terminal split, the "Could not reach the model backend" branch in _format_acp_error, and the llm_helpers._TRANSIENT_MARKERS fallback entries. Suggestion: land #9330 first as the smaller change, then drop _RE_5XX_PHRASE here on rebase.

#9099 (by @iamwhatever) rewrites the entitlement branches of the same _format_acp_error chain and appends a test class at almost the same anchor in test/test_acp_error_surface.py. There is no behavioural conflict between the two, only a mechanical conflict for whichever lands second.

This branch is 170 commits behind main and its mergeable state reads unknown, so a rebase is needed regardless.

Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong.