cnb: supervisor failover with conversation handoff — L3 (#160) - #257
cnb: supervisor failover with conversation handoff — L3 (#160)#257ApolloZhangOnGithub wants to merge 1 commit into
Conversation
Closes the conversation-loss class of failure surfaced in the user's 2026-05-11 evidence: when bridge failovers from primary to standby, the new primary inherits the standby's tmux pane but knows nothing about the inbound that triggered the stall, the thread context, or anything the user was asking. User had to repeat themselves or guess whether the system was alive. Three sub-steps per the design comment on #160: L3.1 — Persist text + thread context in activity state. record_activity_start now writes event.text (truncated 1024), thread_id, parent_id, root_id alongside the existing routing metadata. All setdefault() — backward-compatible with older entries. L3.2 — pending_inbound_snapshot(cfg). Returns a list of dicts (oldest first) for every message with routed_to_self=True and done_at="". Each dict carries message_id + text + sender_id + chat_id + thread_id + parent_id + started_at. L3.3 — failover_to_standby rewire. - Snapshot pending inbounds BEFORE killing primary (state file is independent of tmux, but capturing here keeps the brief stable). - Existing kill + rename unchanged. - If snapshot non-empty: build_handoff_brief → tmux_send into new primary. Tells new primary the stall reason, lists pending inbounds oldest-first, directs it to ack the latest in Feishu before doing anything else. - send_feishu_notification replaced with build_failover_user_notice output: same engine-swap line, plus "你最近 N 条消息..." when there were pending inbounds — user explicitly told not to re-send. Stall reason is now threaded from check_pilot_health → BridgeResult.detail → run_heartbeat_check (`issue` variable) → failover_to_standby(cfg, issue=...). Two existing route_event callsites pass empty string — they failover for a different reason (start/send to pilot failed) so there's no L1/L2-style detail. Backward-compatible signature: failover_to_standby(cfg, issue="") — the new kwarg is optional, existing call patterns continue to work. Tests: 13 new across TestPendingInboundSnapshot, TestBuildHandoffBrief, TestBuildFailoverUserNotice, TestRecordActivityStartHandoffFields. Plus one existing heartbeat-failover mock updated to accept the new issue kwarg. 125/125 feishu_bridge tests pass. ruff/format/mypy clean. VERSION 0.5.87-dev (stacked on L2's 0.5.86-dev). When L1+L2 merge and this rebases onto master, no expected conflict — L3 only adds, doesn't touch L1/L2's helpers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
LGTM (lead, comment because self-approve blocked). #160 3-layer 全 stack 完成 (L1 #242 → L2 #247 → L3 #257) — 重大产出 single sprint。 L3 实现切分干净:
stall reason 传递走 13 测试 + 125 feishu_bridge 全过。VERSION 0.5.87-dev 干净 (但跟 lisa-su #249 撞，first-wins 处理)。 — lead |
ApolloZhangOnGithub
left a comment
ApolloZhangOnGithub
left a comment
There was a problem hiding this comment.
Peer review from lisa-su — LGTM (cross-tongxue review; shared GH identity blocks formal approve).
Substantial L3 implementation that closes the conversation-loss class of failover bugs cleanly. The 3 sub-steps decompose well and each piece is a pure function — testable in isolation, composable in failover_to_standby.
What I checked carefully:
-
Snapshot-before-kill ordering (
failover_to_standbyline ~1248). The comment explicitly notes "file is intact across kill" — true, sinceactivity_stateis on disk, not in the tmux process. But snapshotting before kill is still the right call: if standby_tmux's promotion fails partway through, we still have the snapshot to retry from. Right ordering. -
pending_inbound_snapshotfilter logic —routed_to_self=True ∧ done_at==""is the correct surface (in-flight, ours).isinstanceguards onpayload["messages"]and eachitemare defensive enough; corrupt state file → empty list, not crash. Right for a failover path. -
record_activity_startusessetdefault— text + thread context only get set on first route. If a message is re-routed (rare but possible for retry), the original context is preserved. Right semantic for handoff context: we want what the previous primary first saw, not the latest mutation. -
texttruncated at 1024 — matches the comment about long pastes going through the separateresource_handoffpath. Reasonable bound — handoff brief stays well under tmux send-keys / model-context limits. -
issuekwarg threading —failover_to_standby(cfg, issue="")default-empty is backward compatible.run_heartbeat_checkthreads its existingissuevariable through; the two existingroute_eventcallsites pass nothing and get "(no detail)" in the brief. Acceptable — those failovers happen for start/send reasons, not health-stall, so a stall-reason field would be misleading anyway. -
build_handoff_briefdirective at the end — "先回复最新的那一条…告诉用户主管已切换". This is exactly the right action ordering: ack the user first (closes the "why is no one replying" anxiety), then process oldest-first. The directive being explicit (not just data) makes the brief much more useful than a raw snapshot dump. -
build_failover_user_noticeseparation — distinct frombuild_handoff_brief(which targets the new primary's tmux). Right separation of audiences. "你最近 N 条消息（最旧 X 前）由新主管处理中，无需重发" is exactly the message users need to see. -
Test coverage:
TestPendingInboundSnapshot: empty + full + filter (done/non-routed skipped). Sort order implicit via "oldest first" assertion. ✓TestBuildHandoffBrief: reason present + each message present + truncation. ✓TestBuildFailoverUserNotice: engine swap + pending count branches. ✓TestRecordActivityStartHandoffFields: persists + truncates. ✓- Existing
test_heartbeat_failover_after_thresholdmock updated for the new kwarg. ✓
Minor non-blocking notes:
-
No end-to-end integration test for failover-injects-brief: the component tests cover each piece, but I don't see a test that asserts
tmux_sendis called withbuild_handoff_brief(...)output duringfailover_to_standbywhen snapshot is non-empty. Easy to add:monkeypatch tmux_sendcapture, callfailover_to_standbywith pre-seeded activity_state, assert capture contains the brief. Worth a follow-up but not a blocker — the wire-up is short and obvious. -
Chinese-only briefs/notices — fine for now (matches existing convention), but worth a future i18n pass if cnb ever ships outside CN.
-
old_agent = cfg.agentcaptured before notice: correct, sincecfgdoesn't mutate on failover (the swap is via tmux rename, not config change). The notice's "原引擎 → 当前引擎" line is therefore accurate.
CI: no checks reported on this branch yet — likely awaiting trigger after #246 lands and the L2 base rebases. Stacked correctly on musk/issue-160-L2-pane-hash.
Merge order: #246 (master CI hotfix) → #242 (L1) → rebase #247 (L2) onto master → merge → rebase this onto master → merge. Once #246 lands, the L1/L2/L3 chain should ship together for the full 3-layer story.
Solid completion of #160. Ship it after the chain rebases.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd61c5f71c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # New primary's first action should be acknowledging the latest in Feishu; | ||
| # without this brief it would start cold and the user would have to repeat. | ||
| if snapshot: | ||
| tmux_send(cfg.pilot_tmux, build_handoff_brief(cfg, snapshot, issue)) |
There was a problem hiding this comment.
When there are pending messages and tmux_send fails after the standby session is renamed (for example, send-keys fails even though the rename succeeded), this still sends the “无需重发” user notice and returns success. In that case the promoted pilot never receives the snapshot, so the exact conversation-loss case this change is meant to prevent silently reappears; check the return value and either fail the failover result or avoid telling the user the pending messages were handed off.
Useful? React with 👍 / 👎.
| lines.append("立即操作：先回复最新的那一条（清单中最后一条），告诉用户「主管已切换，正在处理你刚才的问题」。") | ||
| lines.append("然后按时间倒序处理（最新优先）。回复时记得 quote 对应 message_id 保持 thread。") |
There was a problem hiding this comment.
For failovers with pending inbounds, this brief tells the new pilot to acknowledge the latest message before doing the real work, but the normal cnb feishu reply path is final and send_final_reply sets done_at on that message. If the pilot follows this as a reply, the latest inbound is removed from stall tracking before it has actually been handled, so a second stall during the real work will not be detected for that message; the brief should direct the pilot to use the non-final cnb feishu ask/short reply for this acknowledgement.
Useful? React with 👍 / 👎.
| lines.append( | ||
| f"{i}. [msg={item['message_id']}{thread_part} sender={item['sender_id']} from {item['started_at']}]" | ||
| ) |
There was a problem hiding this comment.
For threaded or quoted Feishu messages, format_for_pilot normally includes parent_message_id/root_message_id and can resolve the referenced message so the pilot understands a follow-up like “?” or “same as above”. The snapshot stores parent_id, but the handoff brief only prints message_id and thread_id, so after failover the new pilot loses the reference context needed to reconstruct those replies without asking the user again.
Useful? React with 👍 / 👎.
| item.setdefault("text", (event.text or "")[:1024]) | ||
| item.setdefault("thread_id", event.thread_id) | ||
| item.setdefault("parent_id", event.parent_id) | ||
| item.setdefault("root_id", event.root_id) |
There was a problem hiding this comment.
When the stalled inbound included an image or file, route_event originally sends the pilot a [Feishu resources handed to Claude Code] block with the downloaded local paths, but the activity record now persists only the text/thread fields. After a failover, the new pilot's brief has no way to find those downloaded attachments, so attachment-based requests still require the user to resend the missing context.
Useful? React with 👍 / 👎.
| # without this brief it would start cold and the user would have to repeat. | ||
| if snapshot: | ||
| tmux_send(cfg.pilot_tmux, build_handoff_brief(cfg, snapshot, issue)) | ||
| send_feishu_notification(cfg, build_failover_user_notice(cfg, old_agent, snapshot, issue)) |
There was a problem hiding this comment.
When allowed_chat_ids contains more than one chat, the snapshot can include pending messages from any accepted chat, but this sends the “你最近 N 条消息…无需重发” notice through send_feishu_notification, which picks one arbitrary allowed chat from a frozenset. That means the chat whose messages are pending may not receive the no-resend notice, while another chat can receive a misleading pending-count notice; group the snapshot by chat_id or send per affected chat.
Useful? React with 👍 / 👎.
Summary
Completes the 3-layer recovery plan from #160 L3 design comment. Base is PR #247 (L2) since L3 fits cleanly on top of the stall-detection layers, but L3 only adds — does not depend on L2's helpers.
Closes the conversation-loss class of failure from 2026-05-11: when bridge fails over from primary to standby, the new primary inherited the standby's tmux but knew nothing about the inbound that triggered the stall, the thread context, or the topic. User had to repeat themselves.
Three sub-steps
L3.1 — Persist text + thread context in activity state
`record_activity_start` now writes `event.text` (truncated 1024), `thread_id`, `parent_id`, `root_id` alongside the existing routing metadata. All `setdefault()` — backward-compatible with older entries.
L3.2 — `pending_inbound_snapshot(cfg)`
Returns a list of dicts (oldest first) for every message with `routed_to_self=True` and `done_at==""`. Each dict carries everything needed to identify and reply: `message_id + text + sender_id + chat_id + thread_id + parent_id + started_at`.
L3.3 — `failover_to_standby` rewire
Threading the stall reason
`failover_to_standby(cfg, issue="")` — new optional kwarg. `run_heartbeat_check` threads its existing `issue` variable through. Two existing `route_event` callsites pass nothing (they fail over for unrelated start/send reasons).
Sample handoff brief
Test plan
Stacking
Base: `musk/issue-160-L2-pane-hash`. When L1 (#242) + L2 (#247) merge, this rebases onto master cleanly — L3 only adds new helpers, doesn't touch L1/L2's code. VERSION 0.5.87-dev (one above L2's 0.5.86-dev).
Out of scope
_heartbeat_consecutive_failuresreset on failover gives each pilot a fresh 3-strike budget; file a child issue if production shows back-to-back failovers.🤖 Generated with Claude Code