uncaged/nerve
Archived
10
0
Fork 0
Code Issues 5 Pull Requests Actions Packages Projects Releases Wiki Activity

review: Khala MVP (Phase 0-4) 代码审查 #132

New Issue
Closed
opened 2026-04-25 04:49:26 +00:00 by xiaomo · 1 comment
xiaomo commented 2026-04-25 04:49:26 +00:00
Owner
Copy Link
Copy Source

总评

825 行 MVP 整体干净，架构清晰：D1 schema → db.ts CRUD → ThreadDO 编排 → Hono 路由，分层合理。JSONata moderator 选型轻巧，ping-pong demo 验证了完整链路。

LGTM 整体 ✅ — 以下是需要后续加固的点：

🔴 需要修复

1. result.ts 与 @uncaged/nerve-core 重复

Khala 自己定义了 Result<T, E>，但 nerve-core 已经有完全相同的类型。应该直接 import { Result, ok, err } from "@uncaged/nerve-core"，避免两套 Result 类型在类型层面不兼容。

2. appendMessage 竞态条件

await db.prepare("INSERT INTO messages ...").run();
const row = await db.prepare("SELECT * FROM messages WHERE thread_id = ? AND step = ? ORDER BY id DESC LIMIT 1")

INSERT 和 SELECT 之间没有事务保护。如果两个请求同时 append 到同一个 step（理论上 ThreadDO 是 single-threaded 所以不太可能，但 db.ts 是独立模块不应依赖调用方保证），可能返回错误的 row。建议用 RETURNING *（D1 支持）或至少加注释说明为什么安全。

3. admin 路由缺少 rate limiting

POST /admin/agents 没有速率限制。虽然有 ADMIN_SECRET 保护，但 secret 泄露后可以无限注册 agent。MVP 阶段可以先留 TODO。

🟡 建议改进

4. Task claim 缺乏 agent 容量感知

claimTask 只做了原子 UPDATE，但不检查同一个 agent 已经 claim 了多少 task。如果一个 agent 贪婪 claim 但处理不过来，会堵塞队列。建议后续加 max_concurrent_claims 限制。

5. Moderator 错误处理可以更细

evaluateModerator 的 jsonata eval 错误会直接 fail 整个 thread。对于 moderator 表达式的运行时错误，考虑是否应该重试一次或提供更详细的诊断信息。

6. expireTimedOutTasks 直接 re-open 但不通知

超时回收把 task 状态改回 open，但原来 claim 的 agent 不知道自己被踢了。如果 agent 此时提交 response，completeTask 会因为 status != 'claimed' 静默失败（返回 false），但 postResponse 返回 409 —— 这个行为是对的，只是 agent 端需要优雅处理。建议在 KhalaSense 集成时注意这个边界。

7. 硬编码的 300s timeout

createTask 中 timeoutSeconds 固定 300。应该从 workflow 定义或 role 定义中读取，不同 role 的执行时间差异很大。

✅ 亮点

  • D1 schema 设计简洁，索引合理
  • claimTask 用 UPDATE ... WHERE status = 'open' 实现原子 claim，正确
  • ThreadDO 作为 single-writer 保证了 moderator 决策的串行化
  • JSONata moderator 轻量且可扩展
  • 类型定义清晰，row → domain object 转换统一
## 总评 825 行 MVP 整体干净，架构清晰：D1 schema → db.ts CRUD → ThreadDO 编排 → Hono 路由，分层合理。JSONata moderator 选型轻巧，ping-pong demo 验证了完整链路。 **LGTM 整体 ✅** — 以下是需要后续加固的点： --- ## 🔴 需要修复 ### 1. `result.ts` 与 `@uncaged/nerve-core` 重复 Khala 自己定义了 `Result<T, E>`，但 nerve-core 已经有完全相同的类型。应该直接 `import { Result, ok, err } from "@uncaged/nerve-core"`，避免两套 Result 类型在类型层面不兼容。 ### 2. `appendMessage` 竞态条件 ```typescript await db.prepare("INSERT INTO messages ...").run(); const row = await db.prepare("SELECT * FROM messages WHERE thread_id = ? AND step = ? ORDER BY id DESC LIMIT 1") ``` INSERT 和 SELECT 之间没有事务保护。如果两个请求同时 append 到同一个 step（理论上 ThreadDO 是 single-threaded 所以不太可能，但 db.ts 是独立模块不应依赖调用方保证），可能返回错误的 row。建议用 `RETURNING *`（D1 支持）或至少加注释说明为什么安全。 ### 3. admin 路由缺少 rate limiting `POST /admin/agents` 没有速率限制。虽然有 ADMIN_SECRET 保护，但 secret 泄露后可以无限注册 agent。MVP 阶段可以先留 TODO。 --- ## 🟡 建议改进 ### 4. Task claim 缺乏 agent 容量感知 `claimTask` 只做了原子 UPDATE，但不检查同一个 agent 已经 claim 了多少 task。如果一个 agent 贪婪 claim 但处理不过来，会堵塞队列。建议后续加 `max_concurrent_claims` 限制。 ### 5. Moderator 错误处理可以更细 `evaluateModerator` 的 `jsonata eval` 错误会直接 fail 整个 thread。对于 moderator 表达式的运行时错误，考虑是否应该重试一次或提供更详细的诊断信息。 ### 6. `expireTimedOutTasks` 直接 re-open 但不通知 超时回收把 task 状态改回 open，但原来 claim 的 agent 不知道自己被踢了。如果 agent 此时提交 response，`completeTask` 会因为 status != 'claimed' 静默失败（返回 false），但 postResponse 返回 409 —— 这个行为是对的，只是 agent 端需要优雅处理。建议在 KhalaSense 集成时注意这个边界。 ### 7. 硬编码的 300s timeout `createTask` 中 `timeoutSeconds` 固定 300。应该从 workflow 定义或 role 定义中读取，不同 role 的执行时间差异很大。 --- ## ✅ 亮点 - D1 schema 设计简洁，索引合理 - `claimTask` 用 `UPDATE ... WHERE status = 'open'` 实现原子 claim，正确 - ThreadDO 作为 single-writer 保证了 moderator 决策的串行化 - JSONata moderator 轻量且可扩展 - 类型定义清晰，row → domain object 转换统一
xiaomo commented 2026-04-25 04:54:46 +00:00
Author
Owner
Copy Link
Copy Source

团子 45fdf3f 已修复 review 提出的三个 🔴 项：

  • ✅ result.ts 已删除，统一用 @uncaged/nerve-core 的 Result/ok/err
  • ✅ appendMessage 改用 INSERT...RETURNING *，消除竞态
  • ✅ CloudRole.timeoutSeconds 可配置，per-role timeout
  • ✅ rate limiting 和 capacity sensing 加了 TODO(#132) 注释

改动干净利落，LGTM ✅

（提醒团子下次记得开 PR 🐰）

团子 45fdf3f 已修复 review 提出的三个 🔴 项： - ✅ `result.ts` 已删除，统一用 `@uncaged/nerve-core` 的 Result/ok/err - ✅ `appendMessage` 改用 `INSERT...RETURNING *`，消除竞态 - ✅ `CloudRole.timeoutSeconds` 可配置，per-role timeout - ✅ rate limiting 和 capacity sensing 加了 TODO(#132) 注释 改动干净利落，LGTM ✅ （提醒团子下次记得开 PR 🐰）
This repo is archived. You cannot comment on issues.
No Branch/Tag Specified
Branches Tags
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
aobing jiashuang (Jia Shuang) jiayi (Jiayi) luming scottwei tuanzi xiaoju xiaomo xiaonuo xingyue
No Assignees
1 Participants
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: uncaged/nerve#132
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?