feat(app-server) add app server - #1803
Conversation
84d4ea0 to
c672900
Compare
limityan
left a comment
There was a problem hiding this comment.
PR Review: feat(app-server) add app server
This PR introduces a new bitfun-app-server interface crate providing a protocol-agnostic JSON-RPC server/client scaffold over agent_client_protocol custom roles, and rewires the Server Host WebSocket to use it (browser-direct ACP-over-WS). The overall architecture direction is sound: replacing the custom {type:"request"|...} WS envelope with native JSON-RPC 2.0, centralizing agent kernel + host service operations behind a single schema, and adding ts-rs generated TypeScript bindings. Tests are thorough (round-trip, agent kernel integration, event forwarding, websocket-adapter contract).
However, there are issues that should be addressed before merge:
Blocking
1. SubmitDialogTurnBody::to_request silently drops reply_route and prepended_reminders
schema.rs — The body struct accepts reply_route and prepended_reminders fields from the wire, but to_request() hardcodes them to None / Vec::new():
reply_route: None,
prepended_reminders: Vec::new(),This is silent data loss. If a client sends replyRoute or prependedReminders, they are accepted by deserialization then discarded. Either pass them through (reply_route: self.reply_route, prepended_reminders: self.prepended_reminders) or remove them from the wire body struct so the contract is honest.
2. Missing AGENTS-CN.md file
AGENTS.md line 1 has [中文](AGENTS-CN.md) but the PR does not create AGENTS-CN.md. This is a broken link. Either add the Chinese doc or remove the link.
Should fix
3. PR description is entirely blank
The PR body is the unfilled template — no Summary, Type, Areas, Motivation, Verification, or Reviewer Notes. Per the repo contribution process, at minimum the Verification section should record the commands run (e.g. cargo test -p bitfun-app-server, cargo check -p bitfun-server, pnpm run type-check:web, the websocket-adapter test).
4. log::info! in config handler should be debug or removed
server.rs — The config/getConfig handler logs every request at info level:
log::info!("server getConfig request: {:?}", request);This is noisy in production and logs request details (config paths). Per repo logging rules (src/crates/LOGGING.md), this should be log::debug! or removed.
5. #![recursion_limit] placement in test file is messy
tests/agent_kernel.rs — The #![recursion_limit = "512"] attribute is placed between //! doc comments, with // regular comments breaking the doc block:
//! ... matching `sdk_minimal.rs`. The
// Each `on_receive_request` ...
#![recursion_limit = "512"]
//! other operations ...
Move the attribute to the very top of the file (before all comments) for clarity.
6. gen:types in build scripts couples all web builds to Rust compilation
package.json — build, build:desktop, and build:web now all run npm run gen:types (which runs cargo test --features ts) before vite build. This means every frontend build requires a working Rust toolchain and compiles Rust code. For frontend-only iteration (pnpm run dev:web or CI), this could be a significant slowdown and adds a Rust dependency to the frontend build path. Consider making gen:types a separate opt-in step or only running it in CI/release builds, not every local build:web.
Consider
7. Event forwarder terminates connection on single notification send failure
server.rs serve main loop — If cx.send_notification(notification) returns Err, the loop returns Err, killing the entire connection including any in-flight RPCs. A single failed event push (e.g. transient backpressure) takes down the WebSocket. Consider logging and continuing instead of terminating, at least for non-fatal send errors.
8. {request} envelope unwrapping could misfire
const body = (params && typeof params === 'object' && 'request' in params)
? params.request ?? {}
: params ?? {};If a legitimate request body happens to have a top-level request field, it will be incorrectly unwrapped. The comment acknowledges this is transitional ("Step 2b"), but it's a footgun in the meantime. Consider checking for the exact {request: object} shape more narrowly, or documenting the risk.
9. recursion_limit bumps are a symptom, not a fix
#![recursion_limit = "256"] in lib.rs and main.rs, "512" in tests. Each new on_receive_request handler adds a ChainedHandler layer. As more host-service groups land (the PR comments mention workspace, snapshot, cron, MCP batches), this will need further bumps. This is a known trade-off of the builder pattern — worth tracking as tech debt.
10. Interfaces crate depends on assembly/core with product-full
app-server/Cargo.toml depends on bitfun-core with features = ["product-full"]. The PR acknowledges this mirrors bitfun-acp. It's a pre-existing pattern, not a new violation, but it means the interfaces layer pulls in the full product assembly graph. Worth noting for the architecture record.
Summary
The design is well-documented and the test coverage is good (round-trip, agent kernel, event forwarding, permission missing-port, client connect lifecycle regression). The main concern is the silent field dropping in to_request() (blocking) and the blank PR description. The rest are quality improvements that can follow.
79c38cb to
07c8da1
Compare
There was a problem hiding this comment.
复审结论:请求修改
整体方向与现有设计一致:Server/Web 应作为同级 adapter 复用唯一的 Agent Runtime,App Server 可以作为第一方 rich-client 私有协议;它不应成为第二套 Runtime 或公开 Agent SDK。当前实现主要偏离了既有设计中的“宿主负责协议转换、按能力垂直迁移、真实消费方与版本门槛”原则,合并前需处理以下问题。
1. [阻断] WebSocket 扩大到完整 Runtime 控制面,但没有认证和连接级作用域
问题: /ws 只检查可选 Origin,缺失 Origin 会直接放行;连接没有 initialize/token、workspace、用户或 execution-domain 身份。随后每个连接都能操作全局 Session、取消、权限、Git/config,并订阅同一全局事件与权限流。
风险: 任一本机非浏览器客户端都可直接调用完整控制面;多个 Web client 之间也无法证明 Session、事件和审批归属,可能看到或响应不属于自己的请求。旧 handler 仅暴露 ping/external-sources,本 PR 明显扩大了权限范围。
推荐方案: 在业务消息前完成 fail-closed 的初始化与认证,绑定 connection → user/workspace/session/execution-domain;事件和权限按连接作用域过滤,只允许控制该连接拥有的 Session/Turn。若本阶段只支持单用户本机模式,也应在设计中明确威胁模型和不可跨越的能力上限。
依据:docs/architecture/product-architecture.md:124-134、docs/architecture/agent-runtime-services-design.md:817-825;代码:src/apps/server/src/routes/websocket.rs:29-70、src/crates/interfaces/app-server/src/server.rs:428-546。
2. [阻断] Web adapter 只改方法名,没有完成请求/响应协议转换
问题: start_dialog_turn 仍发送 Desktop/Tauri 的 userInput/imageContexts 并期待 {success,message},Rust schema 要求 message/attachments 并返回 {status,sessionId,turnId}。权限接口还存在 requestId vs request_id、字符串 reply vs tagged object、workspaceId vs project_id;session/config/git 也有裸值与 {sessions}/{configs}/{branches} 包装不一致。适配器当前只解开 {request},没有 codec。
风险: Web 对话、权限审批、配置加载、Session 列表和 Git 分支等生产调用会反序列化失败或读到错误数据形状;绿色测试只验证方法名和独立 Rust schema,没有覆盖真实调用链。
推荐方案: 按命令提供明确的 request encoder / response decoder,或逐个把调用方迁到 owner DTO;增加 AgentAPI/PermissionAPI → WebSocket adapter → Rust schema 的端到端契约测试。不要一次性靠名称表宣称 Desktop/Web 已统一。
依据:docs/architecture/product-architecture.md:146-162;代码:src/web-ui/src/infrastructure/api/adapters/websocket-adapter.ts:38-105,366-385、src/crates/interfaces/app-server/src/schema.rs:86-109,179-279。
3. [阻断/设计偏离] app-server 边界过宽,并提前固化未版本化协议
问题: 新 crate 同时公开通用 role/transport、完整 agent + host-service schema、Rust client 和前端事件投影;其中 AppServerClient/FrontendEvent/connect 仅被测试使用,没有生产消费方。通用 client 内直接把现有 event_name + payload 投影成前端事件,协议没有 initialize/capability、版本、同流 sequence、调用关联、作用域或投递丢失语义。新增生命周期文档还把当前 preview Rust Runtime SDK 描述成“稳定 SDK 接口”。
风险: 单一 Server Host 的私有 DTO 会被过早冻结为通用 API,形成新的大而全后端 facade,并与 SDK Host/公开 Agent SDK 边界混淆;并发、断线和协议演进时,客户端无法识别事件缺口或兼容性。
推荐方案: 先收窄为当前 Server/Web 的真实垂直切片:宿主 DTO、前端事件投影留在 Server adapter;无真实消费方的通用 client/transport 保持内部或删除。为 Server/Web 单独定义版本化事件信封、capability 和丢失/恢复语义;将文档术语改回“Rust Runtime SDK(preview)”。
依据:docs/architecture/product-architecture.md:91-120,136-162、docs/architecture/agent-runtime-services-design.md:118-121,1031-1033、docs/architecture/agent-runtime-deployment-design.md:210-218;代码:src/crates/interfaces/app-server/src/lib.rs:1-14,47-74、src/crates/interfaces/app-server/src/client.rs:1-19,70-110。
4. [阻断] 传输迁移直接移除了现有 external-sources 能力
问题: 旧 Server Host dispatch 被断开,源码已明确说明相关命令现在进入 method_not_found;前端仍原样透传这些命令,没有 capability gate 或 typed unsupported。
风险: 这是已有 Web/Server 能力回归,用户只会得到通用 unknown-command 错误,也违反了逐能力迁移、无法等价时保留兼容边界的要求。
推荐方案: 在 app-server 对应垂直切片完整落地前保留旧 dispatch;或在本 PR 内迁移这些方法,并提供明确的 capability/unsupported 状态和 UI 门控。
依据:docs/architecture/product-architecture.md:106-120,156-162;代码:src/apps/server/src/routes/external_sources.rs:9-16、src/crates/interfaces/app-server/src/server.rs:422-425。
验证
当前 head 07c8da17edf9a237e605c4928367a59551d3c77e:7 项 CI、cargo check -p bitfun-server、cargo test -p bitfun-app-server、边界检查和 git diff --check 均通过;这些结果不能替代真实 Web 调用链、连接隔离和版本化事件契约验证。
Summary
Introduces a new bitfun-app-server interface crate — a protocol-agnostic JSON-RPC server/client scaffold built on agent_client_protocol custom roles (AppServer/AppClient) — and completes the Phase 2 wiring to expose agent-kernel operations and host services to the browser through this single generic surface. The Server Host's WebSocket handler is refactored to drive BitfunAppServer::serve directly via the new ws_transport::ws_lines adapter, replacing the old custom {type:"request"|"response"|"event"} envelope protocol. The browser now connects straight to the in-process app-server over native JSON-RPC 2.0 on the WebSocket — no shared in-process client, no hand-written command routing. The frontend websocket-adapter gains a typed AGENT_COMMAND_SCHEMA mapping that bridges the frontend's snake_case Tauri command names (start_dialog_turn, git_get_status, etc.) to the app-server's agent/, git/, config/* JSON-RPC method names, with request/response type slots generated from the Rust schema.rs via ts-rs.
Type and Areas
Type: Feature
Areas:
src/crates/interfaces/app-server (new crate)
src/apps/server (Server Host WebSocket refactor)
src/web-ui (websocket-adapter typed command mapping + ts-rs export)
src/crates/contracts (ts-rs ts feature additions for schema DTOs)
src/crates/assembly/core (CronService::new coordinator argument)
Motivation / Impact
The BitFun Server Host previously used a custom envelope protocol (WsMessage::Request/Response/Event) over the WebSocket, with a hand-written in-process handle_command router that only supported ping and external_sources. This left agent-kernel operations and host services inaccessible from web mode, and the server followed an entirely different code path from the Desktop host.
This PR unifies the desktop and web surfaces under a single app-server JSON-RPC interface. The browser-facing host interface is the same AgentRuntime SDK operations the Desktop host uses (create/list/delete sessions, submit/cancel dialog turns, permission reply/grants/audit) plus read-only git and config services — all over the same protocol transport. The generic role layer is schema-free, so the scaffold can be reused with a different schema in the future, while the agent/schema/server modules are the Phase 2 wiring that delegates to the bitfun_agent_runtime SDK and bitfun-core service singletons (Option C pattern, mirroring bitfun-acp).
Runtime events and permission lifecycle events are forwarded per-connection as agent/frontendEvent notifications, projected to the browser's existing agentic:// and permission://event channel names, so the frontend listen(...) call sites stay unchanged under browser-direct ACP.
Verification
The corresponding entries in the AGENTS.md verification table are "Shared Rust logic in
core,transport, adapters, or services" ->cargo check --workspaceplus the nearest focusedcargo test, and "Frontend UI, state, or adapters" ->pnpm run type-check:webplus the nearest focused test.Reviewer Notes
Checklist