emrg: stop swallowing AuthError + programming errors in stale check (G129) - #589
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle. Correctly narrows the bare except Exception in check_and_restart_if_stale: only genuinely transient failures (ConnectionRefused/FileNotFound/OSError/JSONDecodeError/TimeoutError/ConnectionClosed) are swallowed; AuthError (token mismatch = config problem) and programming errors now surface to the user. +2 tests verify both propagation paths (AuthError + AttributeError); doc counts 639→641 consistent. CI 31289319716 SUCCESS.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle
Verified locally on PR head (d8e964a): uv run pytest tests/ -q → 641 passed; doc counts synced (639→641 in README.md / README.cn.md / Agent.md, GUI 96 unchanged).
The fix is correct: check_and_restart_if_stale()'s bare except Exception swallowed every failure, hiding the AuthError (token mismatch = config/install problem the user must see — same root-cause family as #583) and masking genuine programming errors. The narrowed tuple keeps only genuinely transient failures (ConnectionRefusedError / FileNotFoundError / OSError / JSONDecodeError / TimeoutError / websockets ConnectionClosed), and the new ConnectionClosed covers mid-ping drops from a daemon dying between handshake and pong. Both new tests are discriminating: AuthError and AttributeError now propagate, while the existing test_server_unreachable_silent confirms transient paths still swallow. No workflow changes.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (3). Third consecutive approval from an independent cycle; merge condition met. AuthError/programming-error propagation verified across two prior reviews; CI SUCCESS.
Summary