Repository navigation
fix(bot-mode): group follow-ups and late replies stay visible (#92003, #105247) - #111283
Conversation
alt-glitch
added
type/bug
Supersedes #92041 (serialize group follow-ups) and #107193 (stranded-reply harvest window, partial #105247), and overlaps #106502 (retry transient member failure). Closes #92003 and #105247 when merged. Linking so reviewers can compare the community PRs against this consolidated fix. |
૮ >ﻌ< ა ci reviewran on d9c3aa2 — fix(bot-mode): show the current thread and newest unresolved
|
kvnloo
left a comment
kvnloo
left a comment
There was a problem hiding this comment.
Approve — the serialization, frozen-boundary watermarks, and failure visibility all check out, and the e2e spec covers the exact reported scenarios.
Two nits, both non-blocking:
- In
queueGroupChatDrive's catch, the drive-levelfailedevent is tagged with the closure'sthread(the send that created the drive), not thenextThreadwhose rounds actually threw — a later queued thread's failure gets attributed to the first thread. Would neednextThreadhoisted out of the loop to fix. - In the collapsed summary,
[...unresolvedFailures.values()].at(-1)follows Map first-insertion order, so A-fails, B-fails, A-fails-again shows B's older failure. Delete-then-set on re-failure would make it recency-ordered.
Serialize room drives through their actual member completion, freeze input watermarks by retained entry identity, and share the same completion path with handoff continuations. Stop discards queued work without releasing an active owner early; rename follows the existing room binding. Observe stranded replies for the hard-cap duration plus grace after the foreground wait, and retain unresolved failures in collapsed Activity. Never automatically retry an ambiguous failed submit within the same drive. Adapted from the queue and boundary approach in #92041 by @enwaiax and harvest-budget approach in #107193 by @Finn763; #106502 by @wadib identified failed-submit watermark consumption. The implementation retains current numeric watermark storage, room lifecycle bindings and serial round limits. Related: #92003, #105247, #100026
Keep failed-member exclusion across the room queue, rather than resetting it per pending thread. A new user action after failure still permits a new attempt. Share the drain activity epoch so a skipped queued thread cannot hide the preceding member failure. Proven red in real Electron: hold transport refusal, enqueue same-thread and cross-thread sends, then release; old head submits three times, fixed head once. Strengthen follow-up evidence with distinct provider replies, exact public log order/count, and per-input inference counts.
Attribute drive-level errors to the thread being drained, not the send that created the queue. Reinsert repeat member failures in recency order so the collapsed activity row cannot show an older sibling failure. Cover both invariants and the repeated-refusal sequence in native Desktop. Thanks to @kvnloo for identifying both review findings.
ac6c73d to
d9c3aa2
Compare
|
Thanks @kvnloo. Both review nits fixed and merged at 9326d9c: queue exceptions now carry the draining thread and repeated failures move to newest unresolved position. Controlled native refusal A/B/A and real queue/malformed-transcript probes verified the findings; final native suite6/6 and Bot Mode units604/604 passed. |
Comment on #111283The caps constants are untouched, so bundle-level cap overrides remain compatible. The sendToGroupChat rewrite changes the minified anchor used by the local slash-command patch. The config-driven follow-up lives in #98616, which reads the three caps from group_chat config with current values as defaults. It is relevant here because the rooms this PR helps most are the first to reach the existing ceiling.
|
4 participants
Bot Mode group follow-ups wait for the active member, preserve unseen messages, and keep late answers and member failures visible.
Late review fixes — verified final head
Head
d9c3aa22f5d70daa5a4ea2e3795ceeb16085555d, rebased onto mainb8bf4843518c4dc1319d4476adda0cc9cc169045, including landed #111240. The additive mock-server resolution preservesgroupScriptedLine, held completions, andreplyForPrompt.Both @kvnloo findings are fixed without new abstractions:
Final committed build: 6/6 native Linux Electron tests passed (2.6m); 604/604 Bot Mode unit tests across 65 files; full Desktop typecheck/lint passed (179 existing warnings, zero errors); production build, Windows-footgun check, compatibility-pointer check, and diff whitespace check passed. The previous 602-unit union plus two new invariants all pass. Native tests cover primary-profile handoff, local Bot Chat handoff, serialized follow-up, late harvesting, repeated refusal, and Stop/resume. The late-harvest clock remains explicitly accelerated; repeated refusal is injected at the actual renderer WebSocket boundary.
npx playwright test e2e/group-turn-integrity.spec.ts e2e/group-to-local-bot-handoff.spec.ts e2e/group-handoff-to-primary-bot.spec.ts --workers=1 --reporter=list npx vitest run src/plugins/hermes-bots npm run check:lint npm run buildReceipts:
/tmp/botmode-campaign/review-late-receipt.json,review-late-native-six.log,review-late-native-red.log,review-late-harvest-red.log,review-late-all-unit.log, andreview-late-lint-final.log. Historical receipts below describe their original heads. Native Windows remains unverified. No merge, watcher, close, or issue comment was performed by this lane.Root cause: a fixed-delay replacement drive could submit into a busy member session, while completion-time watermarks acknowledged input never present in its prompt.
Live repro
Real Electron Desktop, real isolated
hermes servebackends, deterministic loopback inference. No production credentials or private trajectories.The late-reply probe accelerates only the deadline clock and five-second observation timers, while real session.resume RPCs continue to report the held backend busy. It is scheduling-controlled native evidence, not a claim that a seven-minute ordinary busy turn times out after three minutes. The admission-refusal probe injects one explicit transport error; it is not a reproduced production slot-exhaustion signature.
Verified after rebasing onto current main: 5/5 real Electron tests; 600/600 Bot Mode unit tests across 64 files; full Desktop typecheck/lint and production build passed. Lint reports pre-existing warnings outside this change, no errors. Latest independently verified head:
ac6c73d5528d534e4afc87e88dfc787d98cfbef3.Commands:
npm run check:lint npm run build npx playwright test e2e/group-turn-integrity.spec.ts e2e/group-to-local-bot-handoff.spec.ts --workers=1 --reporter=list npx vitest run src/plugins/hermes-botsTwo queue/watermark invariants fail against origin/main: overlapping submit count 2 instead of 1, and trimmed unseen watermark 94 instead of 0. Independent review found automatic retries of ambiguous failed submissions through continuation rounds; a failing regression verified it and the room-queue failed-member exclusion fixes it.
Scope / remaining uncertainty
Related #92003, #105247, #100026. No closing directive for the broad reports.
Credit and overlaps
Adapted @enwaiax's queue/frozen-boundary approach from #92041 and @Finn763's harvest-budget approach from #107193 to the current typed modules. @wadib's #106502 identifies failed-submit watermark consumption; its broad automatic retries are deliberately not adopted. Related overlapping work: #90933, #103298, and thread-session isolation #111003. Preserve the separate handoff fix #111240 during integration (same Desktop version bump; independent mention-routing delta).
Infographic
Independent review follow-up
A failure could be retried by an already-queued thread because the failed-member set reset per drive. A controlled native refusal reproduced three submissions before the correction; the room queue now shares the exclusion until a new post-failure user action. The activity epoch also survives queued threads.
Final committed head rebuilt: five real Electron tests passed (2.5m), 600 unit tests passed across 64 files, full Desktop lint/typecheck passed (179 existing warnings, no errors). Distinct FIRST_REPLY and FOLLOWUP_REPLY each appear once, following both inputs; second inference excludes the first user entry. Additional live rapid-send, rename and disband controls passed. Late observation remains explicitly clock-accelerated; ordinary follow-up, Stop and identity controls use production clocks. Receipts:
/tmp/botmode-campaign/review-a-final-receipt.json,review-a-final-live.log,review-a-identity-final.log.Combined campaign verification
All four exact PR heads (#111240, #111283, #111273, #111298) were locally integrated onto main
1a990f30628c25fb83d29c4d3b3d18dcb085406ein unionee20e99def270fb4c160018358a938e2a344c822; parent verified zero missing commits from every head. Additive test-fixture conflict resolution preserves both group scripting and held-response behavior. 13 native Linux Electron tests passed, covering group handoffs/queue/Stop/late/error, cron owner deferral and custom-root fallback, nested/stale-launcher delivery, parallel side chats, and busy-DM FIFO. 602 Bot Mode unit tests passed; 155 Python invariants passed, 3 skipped; full Desktop build and typecheck/lint passed (179 existing warnings, no errors). Real backend and tool execution with scripted loopback inference; late-observation clocks accelerated only in the named harvest test. Native Windows remains unverified. Evidence and conflict-resolution patch:/tmp/botmode-campaign/union/verification.json,conflicts.patch, and neighboring logs/screenshots. This is local integration proof, not a merge to main.