feat(runtime-host): own bot conversation continuity - #2550
Conversation
|
CI hit the recap session-switch test in the CLI lane. This PR doesn’t touch that path, and the exact test passed 5/5 locally on the current head. I can’t rerun upstream workflows from the fork—could someone rerun the failed job when convenient? |
|
Thanks for taking on this continuity slice. Tracing the crash and reconnect windows here made the remaining ownership boundary much clearer to me. I think this is a good point to pause and reshape the implementation around Runtime Host, rather than keep adding fixes to the Desktop path. Runtime Host now owns durable Session lifecycle and exact Turn admission, so I believe it should also own resolving, creating, and retiring an opaque external-conversation binding in operational SQLite. The bot layer would then only normalize three independent identities: conversation, source event, and reply target. That shape would remove the separate JSON authority, keep backup and restore consistent, avoid the Desktop-side Since the PR has already grown substantially, I worry that fixing the individual findings one by one would leave us with a harder design to maintain. Would you be open to refactoring this around a unified Runtime Host operation and simplifying the Desktop bridge accordingly? I would be happy to review the reshaped version. |
f502ad4 to
f7b73cc
Compare
me2seeks
changed the title
b32238d to
52b9f21
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Codex automated review
I reviewed exact head 52b9f2124ab79f4324a6f391690ec5eb059e5ef6 across remote operation authorization, external-conversation binding, Desktop admission/continuity, queue ownership, transcript recovery, source deduplication, and the individual bot bridges. I found one P1 permission-boundary bypass and two P2 continuity races; see the inline findings. The current test and test_workspaces checks are also failing.
This 46-file PR combines three independently reviewable owners: (1) Runtime Host external-conversation protocol/authority, (2) Desktop bot admission and continuity, and (3) channel-specific bridge identity/ACK behavior. I recommend splitting along those boundaries after fixing the Host-path gate, so each slice can obtain focused security and crash/recovery verification. I did not find a clearly low-value test block to delete.
Disclosure: This is an automated review performed by Codex using delegated adversarial review passes and a final evidence check. It has not been independently verified by Astro-Han or another human reviewer, does not constitute human approval, and does not represent the final judgment of a human reviewer.
52b9f21 to
eb43bb3
Compare
📝 WalkthroughProblem solvedThe PR moves bot conversation continuity into Runtime Host operational SQLite. Runtime Host now owns:
Desktop now normalizes conversation, source-event, and reply-target identities. It submits stable source messages through the canonical Host path. Exact redeliveries recover existing Turns and transcripts without rerunning the model. The PR also fixes Slack thread identity, DingTalk Stream delivery identity, and QQ direct-message reply routing. External conversation identifiers are hashed before persistence. Binding and release-receipt storage is bounded. Source of truthThe PR extends the existing Runtime Host and SQLite operational path. It does not create a parallel JSON authority. Desktop no longer maintains local conversation bindings, source-event deduplication, session limits, explore-mode preparation, or manual retirement reconciliation. Runtime Host owns binding lifecycle and recovery. Solution size and complexityThe changes form one coherent solution. Ownership, protocol, storage, recovery, and channel identity normalization must change together. The new coordinator and protocol surface is necessary to serialize reconciliation, prevent duplicate session creation, recover interrupted claims, reject caller-selected session IDs, release bindings idempotently, purge retired bindings, and recover historical Turns after redelivery. The PR removes obsolete session creation, preparation, invalidation, and unavailable-session paths. Tests replace legacy lifecycle scenarios with coverage for stable routing, replay admission, crash recovery, release, and channel identity handling. No safe deletion is evident beyond the obsolete code and fixtures already removed. Further deletion could weaken coverage for recovery, concurrency, bounded receipts, or channel-specific routing. ValidationCoverage includes:
Reported verification included core, storage, Runtime Host, Desktop, bot, and integration suites, plus formatting and type checks. The reported CLI recap session-switch CI failure was unrelated and passed locally five times. Final required-check status remains unverified from direct evidence. Complexity delta
Total maintenance complexity likely decreases because continuity has one durable owner and fewer cross-process reconciliation paths. The added Host and storage complexity is justified by crash recovery, backup and restore, and idempotent delivery. Review-relevant risks
The person performing the merge must review the final diff. A maintainer makes the final determination. WalkthroughThe change standardizes bot event identity and reply routing, moves conversation-to-session ownership into Runtime Host, adds durable SQLite conversation bindings, and updates desktop turn execution and lifecycle recovery to use reconciliation APIs. ChangesExternal conversation routing
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟠 High · up to This PR changes bot conversation identity and replay handling, but the current head can drop Telegram group or supergroup messages, generate fresh identities for blank source events and duplicate model Turns, return the wrong text for multi-message Turns, leave bindings after archive failure, and persist reversible low-entropy identifiers. These are concrete correctness, delivery, cleanup, and confidentiality risks that should be addressed before merge. Sequence Diagram(s)sequenceDiagram
participant BotBridge
participant BotIncomingMain
participant RuntimeHostClient
participant HostExternalConversationCoordinator
participant SessionCatalog
participant MessageCoordinator
BotBridge->>BotIncomingMain: deliver conversationId and sourceEventId
BotIncomingMain->>RuntimeHostClient: reconcile conversation
RuntimeHostClient->>HostExternalConversationCoordinator: external-conversation.reconcile
HostExternalConversationCoordinator->>SessionCatalog: probe or create session
HostExternalConversationCoordinator-->>RuntimeHostClient: resolved session
BotIncomingMain->>RuntimeHostClient: submit message
RuntimeHostClient->>MessageCoordinator: turn.message.submit
MessageCoordinator-->>BotIncomingMain: turn disposition and state
BotIncomingMain->>BotBridge: send reply through replyTarget
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
packages/storage/src/sqlite-session-metadata-schema.ts (1)
926-927: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDelete the redundant session index.
session_id TEXT NOT NULL UNIQUEon line 922 already creates an implicit unique index onsession_id. That index serves theDELETE FROM external_conversation_bindings WHERE session_id = ?lookup inpurgeSession.external_conversation_bindings_by_sessionduplicates it and only adds write cost.♻️ Proposed simplification
); - CREATE INDEX external_conversation_bindings_by_session - ON external_conversation_bindings(session_id); - CREATE TABLE external_conversation_release_receipts (As per path instructions: "Flag concrete cases where code can be deleted or simplified."
Source: Path instructions
packages/storage/src/__tests__/sqlite-session-metadata-store.test.ts (1)
230-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the literal expected schema version.
The assertion now compares the migrated store against the same constant the migration targets. It passes for every future version bump, so it no longer detects a version bump that ships without its migration. Use the literal version this test intends to reach.
💚 Proposed fix
- assert.equal(migrated.schemaVersion(), SQLITE_SESSION_METADATA_SCHEMA_VERSION); + assert.equal(migrated.schemaVersion(), 26);As per path instructions: "Flag tests that duplicate existing coverage, assert implementation details, or do not protect observable behavior."
Source: Path instructions
packages/core/src/bot-events.ts (1)
166-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove duplicated command matching.
Both functions implement the same group guard and text normalization. Put this logic in one private helper so reset and help cannot diverge.
Proposed simplification
+function isPlaintextBotCommand( + message: Pick<BotMessageEvent, 'text' | 'isGroup'>, + commands: ReadonlyArray<string>, +): boolean { + if (message.isGroup) return false; + const normalized = message.text.normalize('NFC').trim().toLowerCase(); + return normalized.length > 0 && commands.includes(normalized); +} + export function isPlaintextResetCommand( message: Pick<BotMessageEvent, 'text' | 'isGroup'>, ): boolean { - if (message.isGroup) return false; - const trimmed = message.text.normalize('NFC').trim().toLowerCase(); - if (trimmed.length === 0) return false; - return BOT_PLAINTEXT_RESET_COMMANDS.includes(trimmed); + return isPlaintextBotCommand(message, BOT_PLAINTEXT_RESET_COMMANDS); }As per path instructions, "Flag concrete cases where code can be deleted or simplified."
Also applies to: 195-198
Source: Path instructions
packages/runtime/src/bots/__tests__/qq-bridge.test.ts (1)
118-135: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an
isGroupassertion for the direct-message event.
qqDirectMessageToEventforcesisGroup: false. That flag gates the DM-only plaintext reset command. The new case asserts conversation identity and send routing but not this flag, so a regression toisGroup: truewould pass.♻️ Proposed addition
assert.equal(direct?.conversationId, 'dm:channel:dm-channel-1'); + assert.equal(direct?.isGroup, false); assert.deepEqual(direct?.replyTarget, {apps/desktop/src/main/runtime-host-bot-session-adapter.ts (1)
185-258: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
latestMessageIdmakes the completed text depend on delta arrival order.The collector keeps per-message text in
assistantText, but on completion at Line 243 it returns only the entry forlatestMessageId.latestMessageIdis the message of the last observed delta, not necessarily the final assistant message of the Turn. If the Turn emits deltas for two assistant messages and the last delta belongs to the earlier one, the reply text is the wrong message. The same variable also drives thepublishedSnapshotstream, so a two-message Turn can publish alternating snapshots.If a bot Turn can only ever produce one assistant message, then
assistantTextas aMapis unnecessary state and a single string is enough. Either constrain the shape or select the terminal message explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b51fb2e-6862-4377-8da0-19e6613dc280
📒 Files selected for processing (53)
apps/desktop/src/main/__tests__/bot-incoming-project-cwd.test.tsapps/desktop/src/main/__tests__/bot-incoming-session-lifecycle.test.tsapps/desktop/src/main/__tests__/bot-incoming-typing.test.tsapps/desktop/src/main/__tests__/bot-session-adapter-fixture.tsapps/desktop/src/main/__tests__/runtime-host-bot-session-adapter.test.tsapps/desktop/src/main/__tests__/runtime-host-client.test.tsapps/desktop/src/main/__tests__/runtime-host-desktop-candidate.test.tsapps/desktop/src/main/__tests__/runtime-host-session-test-fixture.tsapps/desktop/src/main/bot-incoming-main.tsapps/desktop/src/main/bot-session-adapter.tsapps/desktop/src/main/runtime-host-bot-session-adapter.tsapps/desktop/src/main/runtime-host-client.tsapps/desktop/src/main/runtime-host-desktop-candidate.tspackages/core/src/__tests__/bot-events.test.tspackages/core/src/bot-events.tspackages/runtime-host/src/__tests__/execution-composition.test.tspackages/runtime-host/src/__tests__/external-conversation-coordinator.test.tspackages/runtime-host/src/__tests__/external-conversation-protocol.test.tspackages/runtime-host/src/__tests__/message-coordinator.test.tspackages/runtime-host/src/__tests__/protocol.test.tspackages/runtime-host/src/__tests__/session-catalog-coordinator.test.tspackages/runtime-host/src/__tests__/session-retirement-coordinator.test.tspackages/runtime-host/src/protocol/external-conversation.tspackages/runtime-host/src/protocol/message.tspackages/runtime-host/src/protocol/operations.tspackages/runtime-host/src/server/execution-composition.tspackages/runtime-host/src/server/external-conversation-coordinator.tspackages/runtime-host/src/server/message-coordinator.tspackages/runtime-host/src/server/operation-dispatcher.tspackages/runtime-host/src/server/session-catalog-coordinator.tspackages/runtime-host/src/server/session-retirement-coordinator.tspackages/runtime/src/bots/__tests__/base-adapter.test.tspackages/runtime/src/bots/__tests__/dingtalk-bridge.test.tspackages/runtime/src/bots/__tests__/discord-bridge.test.tspackages/runtime/src/bots/__tests__/im-channel-mapping.test.tspackages/runtime/src/bots/__tests__/qq-bridge.test.tspackages/runtime/src/bots/__tests__/slack-bridge.test.tspackages/runtime/src/bots/__tests__/wechat-bridge.test.tspackages/runtime/src/bots/dingtalk-bridge.tspackages/runtime/src/bots/discord-bridge.tspackages/runtime/src/bots/feishu-bridge.tspackages/runtime/src/bots/qq-bridge.tspackages/runtime/src/bots/slack-bridge.tspackages/runtime/src/bots/telegram-bridge.tspackages/runtime/src/bots/types.tspackages/runtime/src/bots/wechat-bridge.tspackages/runtime/src/bots/wecom-bridge.tspackages/storage/package.jsonpackages/storage/src/__tests__/external-conversation-authority.test.tspackages/storage/src/__tests__/session-store.test.tspackages/storage/src/__tests__/sqlite-session-metadata-store.test.tspackages/storage/src/external-conversation-authority.tspackages/storage/src/sqlite-session-metadata-schema.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for moving external conversation continuity into the Runtime Host. I reviewed this head with three independent @reviewer slices plus a read-only ollama-cloud/deepseek-v4-flash:high pass. The core direction is right: Host storage owns the durable binding, stable source IDs own redelivery, and archive cleanup uses the binding row as a retry marker.
The remaining failures come from not distinguishing three durable states: a claimed Session ID whose Session creation has not committed yet, a live binding, and a binding whose Session is archived/removed and awaits purge. Treating all missing Sessions as stale breaks crash-window idempotency; keeping deterministic create failures forever leaks capacity; and a Desktop pre-check bypasses the Host's existing durable message proof on exact redelivery.
The first-principles fix is an explicit binding/claim lifecycle owned by the storage/Host coordinator, plus one message-admission authority in Host. Preserve unknown-commit claims, release deterministic create failures, purge only archived/removed generations, and let submitMessage return the original turn_started proof before applying busy semantics. Recovery should isolate failures per Session.
This head also predates current main's isArchived-only lifecycle and schema 27. Rebase first, remove the status === 'archived' dependency, and add the binding tables in a monotonic schema 28 migration. Current CI is green but does not test v27 upgrade, crash-window recovery, or exact running-turn redelivery.
No local test suite was run during this review; conclusions are based on source, test, current-main, and CI inspection. Codex coordinated the passes and performed the final adjudication; external-model output was treated as unverified until checked against the code.
中文摘要
感谢把外部会话连续性收敛到 Runtime Host。Host storage 拥有 durable binding、稳定 source ID 负责 redelivery、binding row 作为 archive cleanup retry marker，方向正确。
当前缺口是没有区分三种持久状态：Session 尚未提交的 claim、live binding、以及 Session 已 archived/removed 且等待 purge 的 binding。把所有缺失 Session 都当 stale 会破坏 crash-window 幂等；永久保留确定性 create failure 又会泄漏容量；Desktop 预检查还绕过了 Host 已有的 durable message proof。
最小方案是由 storage/Host coordinator 明确管理 claim lifecycle，并只保留 Host 一处 message admission：保留 unknown-commit claim、释放确定性失败、只 purge archived/removed generation；submitMessage 先返回原 turn_started proof，再应用 busy 语义；recovery 按 Session 隔离失败。
该 head 还早于 current main 的 isArchived-only lifecycle 和 schema 27。请先 rebase，删除 status === 'archived' 依赖，并用单调 schema 28 migration 新增表。当前 CI 全绿，但未覆盖 v27 upgrade、crash-window recovery 和 running-turn 精确重投递。
本次未运行本地测试套件；外部模型输出在核对代码前均视为未验证输入。
547f2c4 to
261cc8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/runtime-host/src/server/session-retirement-coordinator.ts (1)
374-389: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRetire external conversations after a Session removal commit.
The removal path commits the Session deletion but never calls
#retireExternalConversations. The binding can then reference a tombstoned Session until a later Host recovery. It also continues to consume binding capacity.Call the existing callback with
allSessionIdsafter the lifecycle commits and before publication. Add a removal test that verifies the bindings are deleted.Disposition: fix-now.
Proposed fix
removeHandles.scheduledTasks.commit(); archiveHandles?.goal.commit(); archiveHandles?.scheduledTasks.commit(); + await this.#retireExternalConversations(allSessionIds); await this.#graphWake.retireSessions(allSessionIds);As per path instructions, reuse the closest existing seam and make the smallest local correction.
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c3657ee1-7e8d-4795-aeb6-b2821ffdf4a4
📒 Files selected for processing (22)
apps/desktop/src/main/__tests__/bot-incoming-typing.test.tsapps/desktop/src/main/__tests__/runtime-host-bot-session-adapter.test.tsapps/desktop/src/main/__tests__/runtime-host-desktop-candidate.test.tsapps/desktop/src/main/__tests__/runtime-host-session-test-fixture.tsapps/desktop/src/main/runtime-host-bot-session-adapter.tsapps/desktop/src/main/runtime-host-client.tsapps/desktop/src/main/runtime-host-desktop-candidate.tspackages/runtime-host/src/__tests__/execution-composition.test.tspackages/runtime-host/src/__tests__/external-conversation-coordinator.test.tspackages/runtime-host/src/__tests__/protocol.test.tspackages/runtime-host/src/__tests__/session-catalog-coordinator.test.tspackages/runtime-host/src/__tests__/session-retirement-coordinator.test.tspackages/runtime-host/src/server/execution-composition.tspackages/runtime-host/src/server/external-conversation-coordinator.tspackages/runtime-host/src/server/session-catalog-coordinator.tspackages/runtime-host/src/server/session-retirement-coordinator.tspackages/runtime/src/bots/__tests__/wechat-bridge.test.tspackages/runtime/src/bots/wechat-bridge.tspackages/storage/src/__tests__/session-store.test.tspackages/storage/src/__tests__/sqlite-session-metadata-store.test.tspackages/storage/src/external-conversation-authority.tspackages/storage/src/sqlite-session-metadata-schema.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/storage/src/tests/session-store.test.ts
- packages/runtime-host/src/tests/protocol.test.ts
- apps/desktop/src/main/runtime-host-client.ts
- packages/runtime-host/src/tests/session-catalog-coordinator.test.ts
- packages/runtime/src/bots/tests/wechat-bridge.test.ts
- packages/runtime-host/src/server/session-catalog-coordinator.ts
- packages/runtime-host/src/tests/external-conversation-coordinator.test.ts
- packages/runtime-host/src/server/external-conversation-coordinator.ts
- apps/desktop/src/main/runtime-host-desktop-candidate.ts
- apps/desktop/src/main/tests/bot-incoming-typing.test.ts
- packages/runtime-host/src/server/execution-composition.ts
- apps/desktop/src/main/tests/runtime-host-session-test-fixture.ts
- apps/desktop/src/main/tests/runtime-host-desktop-candidate.test.ts
- apps/desktop/src/main/tests/runtime-host-bot-session-adapter.test.ts
- apps/desktop/src/main/runtime-host-bot-session-adapter.ts
- packages/storage/src/external-conversation-authority.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 0 remain after this review.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Moving external conversations onto Runtime Host-owned Session and Turn authority is the correct architectural direction, and deterministic Host operation IDs are the right idempotency seam. Four edge cases currently break that end-to-end contract. In particular, Desktop should not add an earlier volatile dedup authority, and a subscriber must either receive a reset/prefix or initialize from the durable transcript before accepting non-zero offsets.
AI-assisted review disclosure: Codex verified these findings against the current head and traced bot ingress, Host admission, subscription framing, transcript projection, terminal outcome selection, and binding persistence. Two independent reviewer-agent passes and an OpenCode Go DeepSeek V4 Flash (high) adversarial pass were used as inputs. No local tests were run.
中文复核
把外部会话收敛到 Runtime Host 管理的 Session/Turn 权威是正确方向，稳定 operation ID 也是合适的幂等边界。但当前有四个会破坏端到端契约的边缘路径：Desktop 在持久 admission 之前又建立了一层易失去重权威；重连订阅不能消费非零 offset；终态文本可能取到旧消息；创建失败会遗留占用容量的 binding。应优先删除并行去重状态，让订阅和持久绑定生命周期由 Host 契约闭合。
本次为 AI 辅助审查：Codex 在最新 head 上追踪 bot ingress、Host admission、subscription、transcript、终态投影与 binding 持久化；另使用两次独立 reviewer 及一次 OpenCode Go DeepSeek V4 Flash（high）对抗审查。未运行本地测试。
|
/agentic_review |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The Runtime Host-centered shape is now substantially cleaner: durable binding ownership, message admission, crash recovery, and retirement cleanup all converge on the existing lifecycle authority, while the channel layer only projects external identities.
I’m leaving COMMENT because two bounded-edge cases still violate the new idempotency contract: malformed channel payloads can enter without a stable source-event identity, and an evicted reset receipt lets a delayed old reset delete a newer binding. I left focused P2 comments at the responsible boundaries. The smallest coherent fix is to fail closed at adapters that cannot supply a stable source ID, and make release replay incapable of deleting a binding created after the original operation (for example by retaining receipts for an explicit retry horizon or binding the release to the expected generation/session).
I did not treat duplicate ephemeral help/non-text acknowledgements as a blocker: the PR explicitly documents outbound IM delivery as at-least-once, and those paths do not create model Turns.
AI-assisted review disclosure: two Codex reviewers reviewed exact head e2ad534; Codex then independently traced and adjudicated the reported idempotency paths against the current code and PR contract.
中文说明
当前实现已经把 durable binding、消息 admission、崩溃恢复和退休清理统一到 Runtime Host，整体方向正确。仍有两个 P2：缺少稳定 source event ID 的异常平台 payload 会绕过可靠幂等；旧 reset receipt 被淘汰后，迟到的重复 reset 可能删除后来新建的 binding。建议 adapter 在没有稳定 ID 时 fail closed，并让 release 与原 binding generation/session 绑定，或至少按明确的平台重试窗口保留 receipt。
help/非文本提示的重复发送没有列为阻塞，因为 PR 已明确出站 IM 是 at-least-once，而且这些提示不会创建模型 Turn。
e2ad534 to
230f720
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for moving bot conversation identity, admission, replay, and reset receipts into the durable Host authority. The current head is green and the core binding/cleanup route is coherent, but one exact-retry P2 remains inline: the transient burst gate can reject a redelivered event before Host admission gets a chance to recover its durable result.
This PR also changes user-visible Slack threading, QQ direct-message routing, reset feedback, reconnect behavior, and recovery replies. Please add representative bot conversation screenshots for the affected surfaces, including at least threading/reply targeting and reset or reconnect recovery.
The PR body discloses Codex/Maka assistance, but the rendered section contains literal \n escapes and substantive commits 87104669 and 68b5898d lack Generated-by: Maka trailers. Please fix the body formatting and add those trailers, or explicitly confirm those commits were human-only.
Reviewed with Codex as an AI-assisted code review. I verified the exact-head diff, Host admission/replay order, binding cleanup, platform routing, CI, visual evidence, and commit provenance; no external model output was used.
中文说明
把 bot conversation、admission、replay 与 reset receipt 收到 Host 的方向正确，但当前 transient burst gate 在 Host admission 之前执行；重连后批量重投超过 8 条时，第 9 条 exact retry 进不了 durable proof/replay，可能永久丢回复。此 PR 也改变 Slack thread、QQ DM、reset/reconnect 等用户体验，请补代表性 bot conversation 截图。另请修复 PR body 的字面量 \n，并补两个缺失的 Generated-by trailer，或明确为纯人工。
230f720 to
8e75ab7
Compare
|
Review remediation is now on |
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
streamId carries the canonical source-message operation id, so the draft projection and the final send share one identity even before the Runtime Host selects a Turn id. Generated-by: Maka
…entity A redelivery of an id-less message minted a fresh sourceEventId each time, defeating Host admission idempotency and running duplicate Turns. Match the DingTalk bridge and drop the message instead. Generated-by: Maka
placement 'next_turn' + busyBehavior 'reject' either starts the Turn or fails, and an exact redelivery resolves to turn_started through the Host's durable proof. A queued disposition therefore means the submit contract changed; fail closed with a descriptive error instead of misreporting a busy race. Generated-by: Maka
An archive commits the lifecycle before purging external conversation bindings, so a purge failure left stale bindings with no durable retry record — the already-archived path returned success without retrying and recovery only scheduled removal cleanup. The leftover binding rows are themselves the durable record: recovery now lists bound Sessions and retries the purge for any that are archived or gone, and the already-archived path retries the purge before reporting success. Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
Generated-by: Codex
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
packages/runtime/src/bots/qq-bridge.ts (1)
232-239: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDelete the redundant
replyTargetre-assignment.The object spread of
channelalready carriesreplyTarget. The explicit property restates it. Keep the comment, drop the line.♻️ Proposed simplification
return { ...channel, conversationId: `dm:${channel.conversationId}`, // QQ direct messages still use the channel REST route. Only continuity // needs a DM-specific identity; transport must retain the real address. - replyTarget: channel.replyTarget, isGroup: false, };packages/storage/src/__tests__/external-conversation-authority.test.ts (1)
135-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test name promises a bound, but the assertions stop at the limit.
The test seeds
EXTERNAL_CONVERSATION_RELEASE_RECEIPT_TOTAL_LIMIT - 1rows, performs one release, and asserts the total is exactly the limit. It never exercises the state where the total is at the limit. That is the branch wherereleasethrows instead of deleting a binding. Add one more release after this point and assert the rejection, so the global bound is actually protected by this test.Disposition: optional.
apps/desktop/src/main/runtime-host-client.ts (1)
1277-1280: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
listAgentGraphEpochsis outside this PR's stated scope and omits its return type.The PR describes conversation continuity, bot identity, and external conversation bindings. Agent graph epochs are unrelated. If this method belongs to another change, move it there so this diff stays the smallest coherent solution. If it must ship here, add the explicit return type that every neighbouring method on this class declares.
Disposition: optional.
apps/desktop/src/main/bot-incoming-main.ts (1)
246-309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffExtract the admitted-turn path out of
processBotIncomingMessage.
processBotIncomingMessagenow handles help, reset, session resolution, replay probing, rate limiting, reply streaming, the typing loop, and reply delivery in one function of roughly 200 lines. Theadmission_requiredbranch alone spans Lines 246-309 and owns the typingAbortControllerlifecycle. Extract that branch into a named helper that takesturnInput, the reply stream, and the message, and returns the turn result. The rest of the function then reads as a linear flow.Disposition: optional. No behavior change.
packages/runtime-host/src/__tests__/protocol.test.ts (1)
811-814: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an invalid
busyBehaviorassertion.The test covers invalid
admissionMode, but not the independentbusyBehaviorguard. Add the symmetric assertion next to it.packages/runtime-host/src/protocol/operations.ts (1)
205-206: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueMove
agent.graph.epochs.queryto a separate change. The external-conversation coordinator does not use this operation.apps/desktop/src/main/__tests__/bot-incoming-session-lifecycle.test.ts (1)
72-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the test name; this test does not recreate the service.
The test creates one service and never recreates it. The name claims service recreation, which is covered by the test at lines 44-70. The name can lead a maintainer to believe recreation is covered here.
Proposed fix
-test('service recreation replays more than the transient burst before admitting new work', async () => { +test('replays every prior source delivery before admitting new work', async () => {Disposition: optional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c17d7296-5670-4efa-9a8c-9f8811dabb4a
📒 Files selected for processing (32)
apps/desktop/src/main/__tests__/bot-incoming-session-lifecycle.test.tsapps/desktop/src/main/__tests__/bot-incoming-typing.test.tsapps/desktop/src/main/__tests__/bot-session-adapter-fixture.tsapps/desktop/src/main/__tests__/runtime-host-bot-session-adapter.test.tsapps/desktop/src/main/bot-incoming-main.tsapps/desktop/src/main/bot-session-adapter.tsapps/desktop/src/main/runtime-host-bot-session-adapter.tsapps/desktop/src/main/runtime-host-client.tspackages/runtime-host/src/__tests__/execution-composition.test.tspackages/runtime-host/src/__tests__/external-conversation-coordinator.test.tspackages/runtime-host/src/__tests__/message-coordinator.test.tspackages/runtime-host/src/__tests__/protocol.test.tspackages/runtime-host/src/__tests__/session-retirement-coordinator.test.tspackages/runtime-host/src/protocol/index.tspackages/runtime-host/src/protocol/message.tspackages/runtime-host/src/protocol/operations.tspackages/runtime-host/src/server/external-conversation-coordinator.tspackages/runtime-host/src/server/message-coordinator.tspackages/runtime-host/src/server/operation-dispatcher.tspackages/runtime-host/src/server/session-retirement-coordinator.tspackages/runtime/src/bots/__tests__/discord-bridge.test.tspackages/runtime/src/bots/__tests__/im-channel-mapping.test.tspackages/runtime/src/bots/__tests__/qq-bridge.test.tspackages/runtime/src/bots/__tests__/telegram-reply-to.test.tspackages/runtime/src/bots/discord-bridge.tspackages/runtime/src/bots/feishu-bridge.tspackages/runtime/src/bots/qq-bridge.tspackages/runtime/src/bots/telegram-bridge.tspackages/runtime/src/bots/types.tspackages/runtime/src/bots/wecom-bridge.tspackages/storage/src/__tests__/external-conversation-authority.test.tspackages/storage/src/external-conversation-authority.ts
Included review availability: Your plan provides up to 3 included reviews per hour; 0 remain after this review.
Generated-by: Codex
8e75ab7 to
8bd6267
Compare
|
Rebased onto current |
Generated-by: Codex
|
Latest bot-review remediation is on |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed exact head d854710d2bd7763d2a665b474940e319461f2531.
The code findings are resolved: exact historical retries now query the Host in replay_only mode before consuming the transient Desktop rate budget, cannot create new work without durable proof, and only genuinely new admission spends a token. The greater-than-burst restart regression covers the original failure mode. All checks are green, all threads are resolved, and the PR body plus all 27 commits have complete Maka/Codex provenance. I found no remaining P0–P2 code issue.
One acceptance item remains before approval: the linked image explicitly identifies itself as a sanitized deterministic fixture and “not a live workspace capture.” Because this PR changes user-visible Slack/QQ/Telegram threading, reply targeting, and reset/reconnect behavior, please add at least one author-provided, sanitized screenshot from an actual supported bot-channel UI showing the resulting behavior. The fixture is useful documentation, but it does not validate the real platform presentation.
AI-assisted review disclosure: Codex reviewed the exact-head diff, remediation, tests, current CI and threads, visual evidence, and provenance metadata. No external model was used. Astro-Han authorized this review campaign.
中文说明
代码问题已经全部闭环：历史重放会先查 Host 的 durable proof，不会先消耗 Desktop 限流 token；超过 burst 的重启回归也已覆盖。CI、线程和 AI provenance 都完整，没有剩余 P0–P2。
现在只差截图验收：现有图片明确写着它是合成的 deterministic fixture，不是真实 workspace capture。由于本 PR 会改变 Slack/QQ/Telegram 中用户实际看到的 thread、reply target 和 reset/reconnect 行为，请作者补至少一张脱敏的真实受支持 bot channel 截图。
|
Hi — this PR conflicts with current I tested a rebase onto current
These are real source conflicts, so they need your judgement rather than a mechanical rebase — please rebase onto current Thanks for the contribution — happy to help if any conflict is unclear. AI-assisted maintenance note, not a review. It does not count as the required human review under |
2 participants
Summary
Raw external conversation identifiers are converted to fixed-length SHA-256 lookup keys before persistence. This bounds/indexes the key and avoids storing the raw value, but it is not a confidentiality control for low-entropy platform identifiers. Release receipts and live bindings are bounded so an external channel cannot grow operational state without limit.
Outbound IM delivery remains at-least-once because platform send APIs do not share a transaction with Runtime Host state. A crash after a successful send can repeat the reply, but, while the conversation binding remains live, canonical message admission prevents a second model Turn.
Part of #2325
中文说明
外部 conversation identifier 会先哈希再持久化。实时绑定和 release receipt 都有容量上限，外部通道无法无限放大 operational state。
IM 出站投递仍是 at-least-once，因为平台发送 API 与 Runtime Host 状态之间没有共同事务。平台成功收到回复后若进程崩溃，回复仍可能重复发送，但在 conversation binding 仍生效时，标准消息 admission 会阻止第二次模型 Turn。
这是 #2325 的一部分。
Verification
npm --workspace @maka/core test(815 passed)npm --workspace @maka/storage test(777 passed, 14 skipped)npm --workspace @maka/runtime-host test(791 passed)npm --workspace @maka/desktop test(1,130 passed)git diff --checkReview focus
absentversusremovedSession recovery boundary after a binding claimChecklist
Does this PR entail a change in behavior?
Visual evidence
Sanitized deterministic review fixture (not a live workspace capture; no real account data). It shows the affected routing semantics: exact Slack thread/reply targeting, reconnect replay through Host proof without a duplicate Turn, and reset acknowledgement.
AI use
Tool(s) and scope: Maka authored the existing bot continuity implementation, tests, verification, and earlier review remediation. OpenAI Codex authored the replay-before-transient-admission remediation, the latest adapter identity hardening and regression tests, PR documentation updates, and the sanitized visual fixture.
Final squash trailers:
Generated-by: MakaGenerated-by: Codex