fix(acp): classify expired sessions as auth, not transient 5xx - #1984
Conversation
An expired session is rejected with 401/403, but nothing in the ACP error classifier recognised those codes. The aborted request leaves a transport error (DispatchFailure / ConnectionResetError) alongside the rejection, which matched the 5xx family first, so the user was told "transient error — retry in a moment, or switch to a different model". Neither can succeed against an expired login, and the retry ladder burned attempts on it. Adds 401/403 status detection as the primary signal, plus a prose fallback for backends that describe the expiry in words, and routes both through one helper so the message formatter and the retry classifier cannot drift. The message now names the actual remedy and says outright that retrying and model-switching will not help. Genuine 5xx errors stay retryable — covered by a guard test.
Design Review (Fable 5) — ✅ PASSAdvisory design-level review of Design-Verdict: PASS Extends the existing prose-classifier taxonomy in its established shape — shared helper prevents formatter/retry drift, ordering fix targets the actual shadowing root cause. Suggestions
[DESIGN-REVIEWED] ef3e753 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Opus 5 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
…otdev#1984) An expired session is rejected with 401/403, but nothing in the ACP error classifier recognised those codes. The aborted request leaves a transport error (DispatchFailure / ConnectionResetError) alongside the rejection, which matched the 5xx family first, so the user was told "transient error — retry in a moment, or switch to a different model". Neither can succeed against an expired login, and the retry ladder burned attempts on it. Adds 401/403 status detection as the primary signal, plus a prose fallback for backends that describe the expiry in words, and routes both through one helper so the message formatter and the retry classifier cannot drift. The message now names the actual remedy and says outright that retrying and model-switching will not help. Genuine 5xx errors stay retryable — covered by a guard test. Co-authored-by: zejiangg <zejiangg@amazon.com>
Problem / Motivation
When a session expires while the dashboard is open, every subsequent action fails behind a misleading banner:
The UI still looks functional — modals open, Allow responds, the model picker works — but nothing goes through, and the message points at two fixes that cannot possibly work. In the reported case the user stepped away for a couple of minutes, came back to an approval modal, clicked Allow, saw nothing happen, then repeatedly re-asked and swapped models on the app's own advice before realising the session had simply expired.
Root cause
An expired session is rejected with 401/403, and nothing in the ACP error classifier recognised those codes:
_RE_AUTHmatches only Bedrock's named exceptions (AccessDeniedException,ExpiredTokenException, …) — a bare status code carries none of them._RE_5XX_STATUSmatches 5xx only.The aborted request leaves a transport error (
DispatchFailure/ConnectionResetError) next to the rejection, and the 5xx family matched that first. So the failure was classified transient: the user got retry advice, and_is_transient_raw_errorre-armed the retry ladder on an error no retry can clear.What changed
_RE_AUTH_STATUS— detects HTTP 401/403. This is the primary signal, because the rejection arrives with no explanatory wording._RE_SESSION_EXPIRED— prose fallback (session expired,not logged in,login required, …) for backends that describe the expiry in words instead of a code._is_session_expired()— both signals behind one helper, used by both the message formatter and the retry classifier, so the two cannot drift (the same drift hazard the surrounding comments already warn about)._RE_AUTHand before the 5xx family, so a transport error from the aborted request no longer shadows the real cause.kiro-cli login… Retrying or switching models will not help — this is a sign-in issue, not a backend error." Wording matches the existing_NOT_LOGGED_IN_MESSAGE.Testing
473 passedintest/test_acp_client.py;269 passedacrosstest_acp_session_provider.py+test_acp_runtime.py(no regressions). isort / flake8 / mypy clean.Six new/extended tests, mutation-verified against pristine
main— all six fail without the change:test_session_expired_by_http_statusHTTP 401/403/status code 401test_session_expired_401_with_transport_errorDispatchFailure ConnectionResetErrortest_session_expired_rewritetest_session_expired_variantstest_session_expired_with_5xx_token_winstest_session_expired_is_not_transienttest_genuine_5xx_still_transient_with_auth_absentis the overreach guard — it passes onmainas well as here, confirming real 5xx errors stay retryable and the new branch is not swallowing them.Scope
Closes the misclassification half of the issue (item 1). The issue also asks for a "Sign back in" affordance in the UI (item 2) — that is a frontend change and is deliberately not in this PR; the corrected backend message is what unblocks the user today. Filing that separately keeps this diff reviewable.
The branch name mentions #1940 and #1835 — both turned out to be already handled (#1940 merged via #1941, #1835 open as #1841), so this PR contains only the #1942 fix.