fix(desktop): declare a mode for Bot sessions - #4213
Conversation
45a2eba to
a6a5147
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for tracing this through the packaged Bot path. I verified that the problem is real: platform delivery remains healthy, but every Bot conversation fails before Session persistence because the adapter directly requests explore without declaring a product mode.
The fix follows the right authority boundary. A Bot now declares the bot product intent, while the Runtime Host remains the only place that materializes its reserved label and explore permission. That is cleaner than exempting caller-supplied bot labels from validation. Moving the generic mode registry out of deep-research.ts also removes a misleading ownership boundary rather than creating a second registry, and making the mode name optional is the minimum change needed to preserve Feishu/Telegram/WeCom-generated Session names. Deep Research still keeps its Host-owned fixed name.
I found no P0/P1 issues and no user-visible UI/UX change in this diff. The exact head's required test check is green. Approved at a6a5147aaf0801a24718d25dbe9b6c90f7c6ea99; the PR is currently conflicting with main, so it still needs a rebase and exact-head incremental confirmation before merge.
AI-assisted review (OpenAI Codex). I verified the packaged reproduction evidence, create contract, production adapter/coordinator composition, registry ownership, and focused tests.
中文对照
感谢你沿着打包后的 Bot 链路定位问题。我确认问题真实存在:平台收发连接是健康的,但 Bot adapter 直接请求 explore、又没有声明产品 mode,因此所有 Bot 对话都会在 Session 持久化前被 Host 拒绝。
这次修复守住了正确的 authority:Bot 只声明 bot 产品意图,由 Runtime Host 唯一负责生成保留 label 和 explore 权限;这比按调用方传入的 bot label 绕过校验更干净。把通用 mode registry 从 deep-research.ts 移到中立模块,也是在修正 owner,而不是增加第二份 registry。mode name 变为可选,是保留飞书/Telegram/企业微信生成的 Session 名所需的最小变化;Deep Research 仍由 Host 强制使用固定名称。
未发现 P0/P1,也没有用户可见的 UI/UX 变化。当前 exact head 的 required test 已通过。已批准 a6a5147aaf0801a24718d25dbe9b6c90f7c6ea99;但 PR 目前与 main 冲突,仍需 rebase,并在新 head 上做增量确认后才能合并。
本次为 AI 辅助审查(OpenAI Codex)。我核对了打包环境复现证据、create contract、生产 adapter/coordinator 组合、registry owner 和聚焦测试。
89bb2b3 to
9e36938
Compare
|
@Astro-Han All CI checks are green. |
37fa311 to
fc98fd9
Compare
|
Rebased to resolve conflicts @Astro-Han |
fc98fd9 to
904c643
Compare
|
rebased |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the rebase. My approval was on a6a5147; re-checked against 904c643c since a force-push loses reviewer context.
The delta is three files and I found nothing in it:
- Epoch 87 → 88 is correct —
mainis at 87, so there is no collision, and the comment states the incompatibility accurately. renderer/application/contracts/session-start-mode.tsis the right seam, not a way around the ledger. A direct@maka/core/session-start-modeimport from a legacy renderer file failsallowsMigrationDependency, because a package specifier resolves to no renderer target; routing throughapplication/contractsresolves to a public application path, which is what the checker is steering toward. It also matchesapplication/contracts/workhub-request-intent.tsalready onmain, down to being a re-export.renderer-architecture.jsonswaps one dependency path for another in each of the two files, so the total does not grow.
Everything else is byte-identical to what I approved, so the earlier reasoning stands. Re-confirming the two things the rebase could plausibly have broken:
nameis now forwarded unconditionally from the IPC handler, butprepareSessionCreateresolvesmode?.name ?? input.name ?? DEFAULT_SESSION_NAME, so Deep Research still wins its own name andbot— nameless by design — keeps the caller's.resolveCreateSessionRequestdefaultsname, so the value is neverundefinedatdecodeSessionCreateInput'sObject.hasOwncheck.- The "explore requires a declared mode" guard is on creation only — it has been there since #2143 and is what #4193 is hitting, not something this PR adds.
prepareSession'supdateSessionConfiguration({permissionMode: 'explore'})is the deliberate mutation path and is unaffected; for sessions the adapter now creates withmode: 'bot'it simply short-circuits at the=== 'explore'check.
P3 — mode and permissionMode are two axes with one name, and this is the cheap moment to fix it
session.create now carries both mode and permissionMode. A reader meets mode first and takes it for the short form of the other one — and that is not hypothetical. #4193's own title reads "rejects explore sessions without a declared mode", where "explore" is a permission mode and "a declared mode" is a start mode. Reviewing this PR I was asked, by someone who knows this codebase well, why the fix declares mode: 'bot' rather than mode: 'explore' or mode: 'bypass' — the exact conflation, from a careful reader, on the first pass. The reserved label namespace mode:* writes the same ambiguity into persisted data.
The two axes are genuinely different and the table proves it: deep_research and bot map to the same permission while differing in name and labels, so the mapping is intent → boundary and is not reversible. The rule the name should carry is the one in create-session-input.ts — explore is conferred by declaring what the session is, never requested directly.
I raise it here rather than as a follow-up because the cost is asymmetric. mode is a wire field, so renaming it is a protocol change — and this PR is already bumping the epoch to 88 and already editing that field's decoder. Folding the rename in now is nearly free; after bot ships and a third mode arrives, it is a protocol break of its own.
SessionRole would be my first choice — you declare what the session is and the Host grants the boundary, which is exactly the rule — but packages/core/src/session.ts:79 already has SessionRole for the WorkHub coordination session. SessionPreset / preset is unused in the repo and reads correctly beside permissionMode; it undersells the fact that a caller cannot override what the preset confers, which is why I prefer role on the merits.
Scope, so this stays small: rename the type and the field only. The mode:* label prefix is already in persisted sessions and can stay as it is — the two do not have to move together, and I would not want a label migration on this PR.
Entirely your call, and I am not blocking on it. If you would rather keep mode, say so and it goes.
(Aside, not a request: the SessionRole collision looks less like bad luck than a signal. A reserved id, a durable role, and now a start mode all answer "what kind of session is this", and session-store.ts:1269 exists only to assert the first two agree. Worth someone's attention eventually; not this PR's problem.)
Next step
Approval stands. heavy was still running when I finished — merge on a green run of 904c643c.
Evidence boundary: I read the coordinator, the protocol decoder, the IPC handler, and the checker's dependency rule. I did not run the suites locally.
AI-assisted review: drafted with Maka; I verified the name precedence, the explore guard's scope, and the dependency-rule reasoning against source myself.
904c643 to
a5a8e34
Compare
Explore is a boundary a product mode confers, not one a caller may request directly, so the Bot adapter's bare `permissionMode: 'explore'` was rejected and no Bot conversation could open a session. Bot is such a mode, and it carries no name of its own so a Session still reads as the platform that opened it. `session.create.mode` accepts a value it did not before, and a Host that predates it answers `Invalid Session start mode`, so the compatibility epoch moves with it. Generated-by: Claude Code
`sessions:create` dropped the requested name whenever a mode was present, which held only while every mode carried a name of its own. The Host already resolves that precedence, so the name goes to it either way. Generated-by: Claude Code
The registry was born with Deep Research as its only member, so it lived in that module. `bot` is a sibling, not a Deep Research detail, and nobody looks for Bot permissions in a file named for Deep Research. Generated-by: Claude Code
Legacy AppShell dependencies may migrate only behind approved renderer ownership boundaries.
a5a8e34 to
7caa538
Compare
Fixes #4193
Every Bot conversation (Feishu, Telegram, WeCom) answers
Maka 暂时无法处理这条消息:机器人对话处理失败and no session is ever created, while the platform connection itself stays healthy.Root cause:
exploreis a boundary a product mode confers, not one a caller may request directly (create-session-input.ts), but the Bot adapter requested it directly —permissionMode: 'explore'with nomode.prepareCreaterejects that, and the rejection matches no category ingeneralizedErrorMessage, so the Bot shows the generic fallback and the failure reads like a platform or credentials problem. Desktop chat is unaffected: it starts atask.A Bot session is exactly such a product intent, so
botjoinsSESSION_START_MODE_SPECSand the adapter names it instead of asking forexplore. Unlike Deep Research it carries no name of its own —prepareCreateresolvesmode?.name ?? input.name, and a fixed spec name would flatten飞书 任务/Telegram 任务into one label — soSessionStartModeSpec.namebecomes optional. Itsmode:botlabel is reserved for free, sinceprepareCreatealready refuses caller-supplied mode labels.The registry itself moves from
deep-research.tstosession-start-mode.ts. It was born there when Deep Research was its only member;botis a sibling, not a Deep Research detail, and nobody looks for Bot permissions in a file named for Deep Research. Pure move — the importers that only wanted the generic symbols follow the new path.A mode without a name of its own also exposed a sibling defect:
sessions:createdropped the requested name whenever a mode was present, which held only while every mode carried one. It now forwards the name either way and leaves the precedence to the Host. The Bot adapter talks to the Host directly and never took that path, so this is a contract repair, not a second user-visible bug.Deliberately not covered:
bot-incoming-main.tsstill swallowsinvalid_requestinto机器人对话处理失败, which is what made this take a packaged-app patch to diagnose.updateSessionConfigurationstill admitsexplorewith no mode — the pathprepareSessionuses to re-arm a bound Bot session.Before / after, session creation with the input the Feishu Bot actually sends:
Both regressions are guarded where they were rejected — at the coordinator and at the IPC handler. The Bot adapter's own test never reached either: its fake client returns a session without entering
prepareCreate, which is why this shipped.Generative tooling: Claude Code (Opus 5) wrote this change; commits carry a
Generated-bytrailer.