Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Overview
Tiny fix: adds CLOSING state to the WeCom _wait_for_handshake terminal handler. 1-line addition.
Assessment
- Correctness: CLOSING state needed for proper terminal handler state machine coverage.
- Security: No security changes.
- Debug artifacts: None.
Summary
Trivially correct. LGTM.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying the unhandled handshake state. The premise holds on current main: plugins/platforms/wecom/adapter.py:332 omits WSMsgType.CLOSING, while the sibling receive loop handles it at plugins/platforms/wecom/adapter.py:373.
Problems
- The one-line behavior change has no regression test.
tests/gateway/test_wecom.pydoes not exercise_wait_for_handshake()with a CLOSING frame.
Suggested changes
- Add an async fake-WebSocket test that returns
aiohttp.WSMsgType.CLOSINGand asserts the authentication-closeRuntimeError.
The hunk remains unchanged since the PR base (6997dc81); salvaging this fix onto current main should otherwise be mechanical. This is an automated hermes-sweeper review.
| @@ -329,7 +329,7 @@ async def _wait_for_handshake(self, req_id: str) -> Dict[str, Any]: | |||
| if self._payload_req_id(payload) == req_id: | |||
There was a problem hiding this comment.
Please add a regression test with a fake WebSocket returning WSMsgType.CLOSING, asserting _wait_for_handshake() raises the authentication-close RuntimeError. Current tests/gateway/test_wecom.py has no coverage for this handshake state.
953e4c0 to
2f19224
Compare
SummaryThree PRs address #64703 by adding WSMsgType.CLOSING to _wait_for_handshake() so authentication-time closing frames raise immediately; #64716 additionally includes regression tests for CLOSING, CLOSED, and ERROR frames. Related pull requests
Duplicates#64707 and #64727 duplicate the production-code change in #64716; #64716 additionally carries regression coverage. Suggested consolidationKeep #64716 open with a salvage path: preserve its one-line root-cause fix and the regression tests that satisfy the contributor review, while rebasing them onto current main as needed. Keep #64707 and #64727 closed as duplicates of #64716. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I64703(["issue #64703 (open)"])
subgraph Dup64707 ["PRs duplicating each other"]
P64707["PR #64707 (closed)"]
P64716["PR #64716 (open)"]
P64727["PR #64727 (closed)"]
end
P64716 -->|best fix| I64703
class I64703 open
class P64707 closed
class P64716 open
class P64727 closed
class P64716 best
class P64716 target
click I64703 "https://github.com/NousResearch/hermes-agent/issues/64703"
click P64707 "https://github.com/NousResearch/hermes-agent/pull/64707"
click P64716 "https://github.com/NousResearch/hermes-agent/pull/64716"
click P64727 "https://github.com/NousResearch/hermes-agent/pull/64727"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 3 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 6 kB of PR diffs, 7 kB of issue/PR text, 2 kB of discussion (4 comments), 4 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
2f19224 to
ae60193
Compare
_wait_for_handshake() was missing aiohttp.WSMsgType.CLOSING from its terminal state check, causing WeCom reconnection to fail permanently after mass restart when the server sends a CLOSING frame during the authentication handshake. - Adds CLOSING to _wait_for_handshake() terminal handler (1 line) - Adds three regression tests for CLOSING/CLOSED/ERROR frames - All 51 existing wecom tests still pass This mirrors the identical fix already applied to _read_events() in NousResearch#28311 (for issue NousResearch#28293), which was overlooked in _wait_for_handshake(). Fixes: NousResearch#64703
ae60193 to
5baa746
Compare
What
Adds
aiohttp.WSMsgType.CLOSINGto the terminal state check in_wait_for_handshake(), matching the identical fix already applied to_read_events()in #28311 (for issue #28293).Why
_wait_for_handshake()was missingWSMsgType.CLOSINGfrom its terminal state handler, causing WeCom reconnection to fail permanently with "closed during authentication" when the server sends a CLOSING frame during the authentication handshake.When #28311 fixed
_read_events()for issue #28293,_wait_for_handshake()was overlooked — both functions handle WebSocket messages and need to recognize all terminal states.Impact
In production with multiple WeCom agents: after a mass restart event, agents that receive a CLOSING frame during the reconnection handshake get stuck in a permanent failure loop — manual pod restart is required to recover.
Fix
One-line change in
plugins/platforms/wecom/adapter.py:Identical to the pattern already in
_read_events()(line ~370).Fixes: #64703