Skip to content

feat: multi-runtime support (OMP, Claude SDK, BitFun Native) — merged…#931

Closed
AHCzyz wants to merge 3 commits into
GCWing:mainfrom
AHCzyz:master
Closed

feat: multi-runtime support (OMP, Claude SDK, BitFun Native) — merged…#931
AHCzyz wants to merge 3 commits into
GCWing:mainfrom
AHCzyz:master

Conversation

@AHCzyz
Copy link
Copy Markdown

@AHCzyz AHCzyz commented May 28, 2026

… onto upstream main

AgentRuntime trait system, RuntimeRegistry singleton, three adapters (BitfunRuntime in-process, OmpRuntime subprocess, ClaudeRuntime Node.js bridge), frontend RuntimeSelector, per-session runtime persistence.

Merged from multi-runtime-support branch onto upstream GCWing/BitFun latest (e4d26e0). Upstream mode config renaming preserved alongside new runtime commands.

… onto upstream main

AgentRuntime trait system, RuntimeRegistry singleton, three adapters
(BitfunRuntime in-process, OmpRuntime subprocess, ClaudeRuntime Node.js
bridge), frontend RuntimeSelector, per-session runtime persistence.

Merged from multi-runtime-support branch onto upstream GCWing/BitFun
latest (e4d26e0). Upstream mode config renaming preserved alongside
new runtime commands.
@GCWing GCWing requested a review from wsp1911 May 29, 2026 00:05
@GCWing
Copy link
Copy Markdown
Owner

GCWing commented May 29, 2026

The direction is absolutely right. Introducing a pluggable Agent Runtime layer is a valuable architectural move for BitFun: it separates product/session surfaces from concrete execution engines, and it opens the door to OMP, Claude Agent SDK, BitFun Native, and future runtimes behind one contract.

Review Findings

[P0] Runtime selection is not wired into actual dialog execution
The UI persists runtimeId, but the main chat path still calls startDialogTurn without passing runtime information, and the backend request does not include it either. The turn then continues through the existing scheduler / ExecutionEngine::execute_dialog_turn path. So selecting OMP or Claude in the UI currently does not make the session execute through OMP or Claude.

[P1] The PR currently fails frontend type-check
CI fails in src/web-ui/src/app/scenes/agents/hooks/useAgentsList.ts because ModeInfo is used with configProfileId, configProfileLabel, and configProfileMemberModeIds, but those fields are not present on the declared ModeInfo type. This blocks merge.

[P1] Session creation drops runtime configuration
create_session constructs SessionConfig with runtime_id: None, even when config is provided. If runtime is meant to be a session-level choice, this should be accepted and persisted during session creation, not only patched afterward through update_session_runtime.

[P2] Claude runtime availability can mislead users
ClaudeRuntime::health_check only verifies the bridge script, while Node.js and ANTHROPIC_API_KEY are checked later during session creation. The UI may show Claude as available even though it will fail on first use. Either include these checks in health status or expose a clearer “configured/unconfigured” state.

[P2] RuntimeSelector allows selecting unavailable runtimes
Unavailable runtimes are styled differently, but handleSelect still accepts them. Users can select an unavailable runtime and then get locked after the session starts. The selector should disable unavailable options or require an explicit unsupported-state path.

Overall
Strong architectural direction, but the PR is not ready to merge yet. The main missing piece is the runtime dispatch loop: selected runtime must flow from frontend/session config into backend execution, and execution must actually call the selected AgentRuntime. After that, fix the type-check failure and tighten availability/selection behavior.

Copy link
Copy Markdown
Contributor

@harryfan1985 harryfan1985 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review:多运行时支持(OMP / Claude SDK / BitFun Native)

读完整 PR 全部 diff(29 文件,+2493/−12)并对照现有代码后,整理如下。结论先行:抽象层设计合理且有价值,但目前作为一个 feature 不可工作,且存在若干确定性 bug。建议拆分或在本 PR 内补齐执行链路后再合并。


🔴 阻塞级:运行时选择根本没接进执行路径

这是最关键的问题。

  • get_global_runtime_registry() runtime_api.rslist_agent_runtimes / get_default_agent_runtime 里被调用,二者仅做健康检查并返回 DTO 给 UI。
  • 三个适配器的 create_session() / prompt() / steer() / abort() 在整个 diff 中从未被任何执行代码调用
  • runtime_id 被存入 SessionConfig 并持久化,但后端在发起一轮对话时没有任何地方读取 runtime_id 去路由到对应运行时;真正的聊天仍 100% 走原有 BitFun coordinator。

结果:用户在 UI 选 "OMP" / "Claude" 除了改一个存储字段和药丸文字外,对 agent 行为没有任何影响。 OmpRuntime / ClaudeRuntimeprompt() 目前是死代码。

🔴 阻塞级:双重初始化 → 启动 panic 风险

init_global_runtime_registry() 第二次调用会 panic(expect("RuntimeRegistry already initialized"))。但 diff 中有两处注册:

  • 核心 system.rs::init_agentic_system()
  • 桌面 lib.rs::init_agentic_system()(自行构造 AgenticSystem { .. } 再注册一遍)

若两条初始化路径在桌面启动时都执行(很可能),应用会在启动时直接 panic。两份注册逻辑几乎重复,至少一份多余,请确认实际调用图。

⚠️ 高危:疑似误删上游 ModeInfo 字段

AgentAPI.ts 删除了 ModeInfoconfigProfileId / configProfileLabel / configProfileMemberModeIds 及对应 fallback 赋值。PR 描述称"上游 mode config 重命名已保留",但 diff 实际是删除。这是一个 feature 分支 merge 到上游的大 PR,这很可能是把上游新增字段误回退,会破坏其它 ModeInfo 消费方。请人工确认这不是 merge 事故。


确定性 bug(适配器层)

1. OmpSession 多轮对话死锁/串话
prompt() 每轮 spawn_reader,reader 在 task 内 stdout.lock().await 后持锁循环直到 EOF/abort。第二轮 prompt() 的新 reader 永远拿不到 stdout 锁 → 死锁;即便不死锁,第一轮 reader 会吞掉后续所有轮次输出并发往已 drop 的 channel。OMP 多轮交互是坏的。

2. ClaudeSession 设计自相矛盾

  • 单个共享 event_tx: Arc<Mutex<Option<Sender>>> 每轮替换,无轮次边界隔离,上一轮迟到事件会泄漏进下一轮 channel。
  • abort() 直接 child.kill() 杀掉整个 Node 进程,但这是"每会话长生命周期进程"模型 —— abort 一次后会话即废,无法复用。
  • bridge.mjs 每次 prompt 都全新 query()、不传 session/resume,多轮上下文不保留,Claude 运行时实际每轮无状态。

3. bridge.mjs tool_call id 不匹配
tool_call_start 用真实 tool id;input_json_deltatool_call_delta 却用 ev.index(下标字符串)。二者对不上,前端无法把参数增量关联到对应工具调用。另:const block = ev.content_block || ev.index != null ? ev : null; 运算符优先级混乱,建议重写。

4. 健康检查误报"可用"
ClaudeRuntime::health_check 只检查 bridge 脚本存在,node / ANTHROPIC_API_KEY 要到 create_session 才查。结果 UI 显示绿点"可用",点下去才失败 —— UX 陷阱。


构建 / 打包

5. Claude bridge 的 npm 依赖无安装步骤
package.json 声明 @anthropic-ai/claude-agent-sdk,tauri.conf.json 将目录列为资源,但没有 npm install 构建步骤,也未 vendoring node_modules。打包后 bridge 会 ERR_MODULE_NOT_FOUND,该路径在产物里无法运行。

6. OMP 二进制实质永不可用
resources/omp/*.exe 被 gitignore,仓库仅 .gitkeep 占位。默认优先级排第一的 OMP 实践中几乎永远"不可用",一切回落到 bitfun。


一致性 / 质量

  • 三套互相不一致的默认运行时策略:前端优先 omp;后端 get_default_agent_runtime 取第一个 available(注册顺序下=bitfun);RuntimeRegistry::select_default 为 omp→claude→bitfun(且实际无人调用)。
  • BitFun 能力硬编码三份:Rust fallback_runtimesBitfunRuntime::new、TS BITFUN_RUNTIME —— 易漂移。
  • update_session_runtime 不校验 runtime_id 是否为已注册/可用运行时。
  • 调试日志残留:runtime_api.rs 大量 log::info!RuntimeSelector.tsxconsole.log(JSON.stringify(...)),建议降级/移除。

测试

仅 registry 的 3 个 mock 单元测试。缺少适配器事件翻译、子进程生命周期、bridge 翻译及端到端验证。鉴于核心功能未接通,现有测试几乎只覆盖脚手架本身。


建议

runtime-ports 的 trait 抽象、统一事件模型、registry 设计本身合理、有价值。但当前 PR:① 运行时选择对执行无效果;② 启动期双重 init panic 风险;③ OMP 多轮死锁 / Claude abort 后失效 / bridge id 不匹配等确定 bug;④ Claude/OMP 在打包层面跑不起来;⑤ 疑似误删上游 ModeInfo 字段。

两条路径供选择:

  • (a) 拆分:仅合"抽象层 + registry(标注 internal/未启用)",执行分发与各适配器后续单独成 PR;
  • (b) 在本 PR 内补齐 coordinator 按 runtime_id 路由的执行链路,修掉 OMP/Claude 并发 bug、补打包步骤与集成测试。

当前状态不建议作为可用功能合并。

@AHCzyz AHCzyz closed this May 30, 2026
@AHCzyz AHCzyz deleted the master branch May 30, 2026 03:23
@harryfan1985
Copy link
Copy Markdown
Contributor

建议关闭本 PR —— 多运行时应复用现有 ACP 子系统,而非另造一套

在前一条 review 指出代码层面的阻塞问题(运行时选择未接进执行、双重 init panic、OMP 多轮死锁、bridge 协议/打包等)之后,我又把仓库现有能力和 OMP 本身查清楚了,结论是:本 PR 的整体方向与 BitFun 已有的 ACP 子系统高度重复,且实现更弱。建议关闭,改走 ACP。

1. BitFun 早已是一个"多运行时"系统 —— 通过 ACP

src/crates/acp 里有完整成熟的 ACP client,已经把外部自治 agent 当作 flow_chat 会话来跑:

  • 会话即普通会话,agent_type = "acp:<clientId>"(session_persistence.rs:45、前端 flow_chat/utils/acpSession.ts),同一聊天 UI、同一 transcript 渲染。
  • 已内置 opencode / claude-code / codex(client/builtin_clients.rs)。
  • 完整命令面已就绪(acp_client_api.rs):create_acp_flow_sessionstart_acp_dialog_turncancel_acp_dialog_turnget/set_acp_session_modelget_acp_clientsinstall_acp_client_clipredownload_acp_client_adaptersubmit_acp_permission_response、远程连接等。
  • 事件已双向映射(runtime/events.rs:ACP SessionUpdateToolEventData),工具路由 bash→terminal/createread→fs/read_text_file、edit→session/request_permission(走 BitFun 权限)。

本 PR 新引入的 AgentRuntime/RuntimeRegistry/三个子进程适配器/RuntimeSelector,几乎是把上面这套东西平行重写了一遍 —— 而且功能更少(工具只读、无安装/探测、无权限集成、无远程)。

2. 本 PR 的两个目标运行时,ACP 都已覆盖

  • Claude:已经通过 claude-code(npx @zed-industries/claude-code-acp)接好了。本 PR 的 resources/claude-bridge/bridge.mjs + @anthropic-ai/claude-agent-sdk 是在重造已有轮子(且无 npm install 步骤、打包跑不起来)。
  • OMP (oh-my-pi):它原生支持 omp acp(Agent Client Protocol over JSON-RPC,见其 README)。而本 PR 走的是自定义 omp --mode rpc,且协议字段是错的 —— 真实 schema 是 {"id":"r1","type":"prompt","message":"..."},本 PR 写成了 {"command":"prompt","text":"..."}(字段名、id 全对不上,steer 命令在 RPC 模式里根本不存在)。这进一步印证了该适配器从未真正对着 omp 跑通。

3. 正道:OMP 只需一条 ACP preset

接 OMP 的代价不是"新写一个 OmpRuntime + 修并发 bug + 打包二进制",而是:

// builtin_clients.rs
BuiltinAcpClientPreset {
    id: "omp",
    command: "omp",
    args: &["acp"],
    tool_command: "omp",
    install_package: "@oh-my-pi/pi-coding-agent",
    adapter_package: None,
    adapter_bin: None,   // 和 opencode 一样,native ACP 无需 adapter
},

加上前端设置目录里的一项即可 —— per-workspace 的 ACP 会话菜单是动态从 get_acp_clients 读的,OMP 会自动出现。我已在本地按此实现并验证(cargo test -p bitfun-acp 45 passed、相关 vitest 10 passed),改动仅 2 文件 +40/−1,对比本 PR 的 29 文件 +2493。

结论

多运行时这个目标是好的,runtime-ports 的 trait 抽象本身也不丑;但在 BitFun 已经有 ACP 这条标准、成熟、已集成多个外部 agent 的通道的前提下,再平行造一套自定义子进程适配器,是重复建设 + 维护负担 + 已知 bug。

建议:关闭本 PR,OMP/Claude 等外部 agent 统一走 ACP(OMP 加 preset,Claude 已具备)。如果未来确有"不会说 ACP、也无 ACP 适配器"的运行时需要接,再单独讨论一个轻量抽象也不迟,但不应以本 PR 的形态进 main。

@AHCzyz
Copy link
Copy Markdown
Author

AHCzyz commented May 30, 2026

Indeed, ACP is enough. So yesterday I closed the PR.

@harryfan1985
Copy link
Copy Markdown
Contributor

进展同步:已提交替代方案 #963

接续前面的分析(建议关闭本 PR、OMP/Claude 改走现有 ACP 子系统),我已经把 OMP 的 ACP 接入实现出来并开了 PR:#963

要点:

  • OMP 原生支持 omp acp,所以只需在 builtin_clients.rs 加一条 preset + 前端目录一项,即复用整套已验证的 ACP 能力(子进程/JSON-RPC/工具路由/权限/会话持久化/模型选择)。
  • 按"BitFun 只提供接入、不负责安装"处理:OMP 面向 bun、自带安装器,故设为用户自管(install_package=None,隐藏一键安装按钮),只检测 PATH 上的 omp
  • 改动量对比:feat(acp): add Oh My Pi (omp) as a built-in ACP agent #963 约 4 文件 +64/−8 vs 本 PR 29 文件 +2493,且无自定义子进程协议、无打包问题、无并发 bug。
  • Claude 这边无需新 PR —— 已有 claude-code(npx @zed-industries/claude-code-acp)ACP preset 覆盖。

因此本 PR 的目标已由 #963(OMP)+ 现有 claude-code(Claude)达成。维持此前结论:建议关闭本 PR,多运行时统一走 ACP。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants